|
suites.All ran without related failures on 10.4:
Tests run: 10340, Failures: 1, Errors: 0 The failed test was the ManagementMBeanTest.testStartStopManagementFromApplication test. 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.
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. '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! 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. > 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. 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 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Sub-task
Closed
Minor
I'm running the regression tests and hope to get this patch in today.
Patch ready for review.