Details

      Description

      There are currently two identical interfaces called EngineBlob and EngineClob.
      Merging these into a single interface would simplify some of the code handling Blob and Clob in the network server, and it also allows a leaner implementation of a stored procedure freeing LOB locators without knowing if the locator represents a Blob or a Clob.

      1. derby-3576-1a-enginelob_interface.diff
        9 kB
        Kristian Waagan
      2. derby-3576-1a-enginelob_interface.stat
        0.5 kB
        Kristian Waagan
      3. derby-3576-1b-enginelob_interface.diff
        9 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        'derby-3576-1a-enginelob_interface.diff' merges the two interfaces into one, and updates all references and usages.

        I'm running the regression tests and hope to get this patch in today.
        Patch ready for review.

        Show
        Kristian Waagan added a comment - 'derby-3576-1a-enginelob_interface.diff' merges the two interfaces into one, and updates all references and usages. I'm running the regression tests and hope to get this patch in today. Patch ready for review.
        Hide
        Kristian Waagan added a comment -

        suites.All ran without related failures on 10.4:
        Tests run: 10340, Failures: 1, Errors: 0

        The failed test was the ManagementMBeanTest.testStartStopManagementFromApplication test.

        Show
        Kristian Waagan added a comment - suites.All ran without related failures on 10.4: Tests run: 10340, Failures: 1, Errors: 0 The failed test was the ManagementMBeanTest.testStartStopManagementFromApplication test.
        Hide
        Knut Anders Hatlen added a comment -

        I had a quick look at the patch and I think it reduces the complexity of the code, so it gets a +1 from me.

        Show
        Knut Anders Hatlen added a comment - I had a quick look at the patch and I think it reduces the complexity of the code, so it gets a +1 from me.
        Hide
        Bryan Pendleton added a comment -

        I have no objections to the patch as proposed, but am wondering: would there
        be any benefit to handling this differently, by introducing a "EngineLargeObject"
        interface from which both EngineBlob and EngineClob extend, and moving all
        of the current content of both EngineBlob and EngineClob into EngineLargeObject?

        That way, code such as you mention, which doesn't need to know the difference
        between the two, could be simplified by just using EngineLargeObject, but
        code (perhaps in the future) which needed to handle Clob differently than Blob
        could still use one of the sub-interfaces.

        Just thought I'd raise the possibility; as I said before I have no objections to the
        proposal as stated.

        Show
        Bryan Pendleton added a comment - I have no objections to the patch as proposed, but am wondering: would there be any benefit to handling this differently, by introducing a "EngineLargeObject" interface from which both EngineBlob and EngineClob extend, and moving all of the current content of both EngineBlob and EngineClob into EngineLargeObject? That way, code such as you mention, which doesn't need to know the difference between the two, could be simplified by just using EngineLargeObject, but code (perhaps in the future) which needed to handle Clob differently than Blob could still use one of the sub-interfaces. Just thought I'd raise the possibility; as I said before I have no objections to the proposal as stated.
        Hide
        Kristian Waagan added a comment -

        'derby-3576-1b-enginelob_interface.diff' is the same as 1a, except a comment is added to the EngineLOB class JavaDoc.

        Bryan, what you describe is what I had in mind as well.
        Unless I have misunderstood you, the difference between my current patch and your suggestion is that my patch removes the two unused interfaces whereas you're considering to keep them.

        My thought was that since the two interfaces will be empty and there is no other code needing them, it was better to get rid of them. Introducing them later will take very little effort, and we also get the possibility of a discussion on whether they are required or not.
        My main motivation behind the change in the first place, was that separating between Blob and Clob is not currently required and it complicates other work on the client.

        For the record, large object handling through the interfaces discussed is very limited. They were introduced to support locators.

        Normally I would wait for your reply on this one, but because the release candidate is getting very close and you say you have no objections to the current patch, I push forward and commit it.
        Thank you for your comment!

        Show
        Kristian Waagan added a comment - 'derby-3576-1b-enginelob_interface.diff' is the same as 1a, except a comment is added to the EngineLOB class JavaDoc. Bryan, what you describe is what I had in mind as well. Unless I have misunderstood you, the difference between my current patch and your suggestion is that my patch removes the two unused interfaces whereas you're considering to keep them. My thought was that since the two interfaces will be empty and there is no other code needing them, it was better to get rid of them. Introducing them later will take very little effort, and we also get the possibility of a discussion on whether they are required or not. My main motivation behind the change in the first place, was that separating between Blob and Clob is not currently required and it complicates other work on the client. For the record, large object handling through the interfaces discussed is very limited. They were introduced to support locators. Normally I would wait for your reply on this one, but because the release candidate is getting very close and you say you have no objections to the current patch, I push forward and commit it. Thank you for your comment!
        Hide
        Kristian Waagan added a comment -

        Committed 'derby-3576-1b-enginelob_interface.diff ' to trunk with revision 642707 and merged it to the 10.4 branch with revision 642711.
        Unless the ongoing discussion brings out the need for more changes, I don't expect more patches to go in for this issue.

        Show
        Kristian Waagan added a comment - Committed 'derby-3576-1b-enginelob_interface.diff ' to trunk with revision 642707 and merged it to the 10.4 branch with revision 642711. Unless the ongoing discussion brings out the need for more changes, I don't expect more patches to go in for this issue.
        Hide
        Bryan Pendleton added a comment -

        > better to get rid of them. Introducing them later will take very little effort,
        > and we also get the possibility of a discussion on whether they are required or not.

        I agree. I looked more closely at derby-3576-1b-enginelob_interface.diff and it
        looks excellent. I see no reason to have the empty interfaces until we are ready
        to use them.

        Thanks for the follow-up, Kristian.

        Show
        Bryan Pendleton added a comment - > better to get rid of them. Introducing them later will take very little effort, > and we also get the possibility of a discussion on whether they are required or not. I agree. I looked more closely at derby-3576-1b-enginelob_interface.diff and it looks excellent. I see no reason to have the empty interfaces until we are ready to use them. Thanks for the follow-up, Kristian.
        Hide
        Mamta A. Satoor added a comment -

        Kristian, one minor thing I noticed is javadoc gives following error on our internal IBM machines. Could it be because javadoc does not know where to find the class EmbedConnection? It seems strange but do we need to have an import of this class in order for javadoc to find the link to it?
        Docs:
        Failed with following errors. Check details in docs.txt
        [javadoc] C:\nightlies\main\src\opensource\java\engine\org\apache\derby\iapi\jdbc\EngineLOB.java:46: warning - Tag @link: reference not found: EmbedConnection#getLobMapping
        [javadoc] C:\nightlies\main\src\opensource\java\engine\org\apache\derby\iapi\jdbc\EngineLOB.java:46: warning - Tag @link: reference not found: EmbedConnection#getLobMapping
        [javadoc] 2 warnings

        Show
        Mamta A. Satoor added a comment - Kristian, one minor thing I noticed is javadoc gives following error on our internal IBM machines. Could it be because javadoc does not know where to find the class EmbedConnection? It seems strange but do we need to have an import of this class in order for javadoc to find the link to it? Docs: Failed with following errors. Check details in docs.txt [javadoc] C:\nightlies\main\src\opensource\java\engine\org\apache\derby\iapi\jdbc\EngineLOB.java:46: warning - Tag @link: reference not found: EmbedConnection#getLobMapping [javadoc] C:\nightlies\main\src\opensource\java\engine\org\apache\derby\iapi\jdbc\EngineLOB.java:46: warning - Tag @link: reference not found: EmbedConnection#getLobMapping [javadoc] 2 warnings
        Hide
        Kristian Waagan added a comment -

        Thanks for informing me about the problem Mamta.

        I have fixed it with revision 643326 and 643328 (10.4), and you are correct the we either need an import or use a fully qualified reference to the class. I chose the latter, since we don't use EmbedConnection in the method signatures.

        Closing the issue.

        Show
        Kristian Waagan added a comment - Thanks for informing me about the problem Mamta. I have fixed it with revision 643326 and 643328 (10.4), and you are correct the we either need an import or use a fully qualified reference to the class. I chose the latter, since we don't use EmbedConnection in the method signatures. Closing the issue.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development