Derby
  1. Derby
  2. DERBY-796

jdbc 4.0 specific Blob and Clob method support

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.2.1.6
    • Component/s: JDBC
    • Labels:
      None
    • Environment:
      jdbc 4.0 on all platforms
    1. lob_6.diff
      408 kB
      V.Narayanan
    2. lob_6.stat
      3 kB
      V.Narayanan
    3. ClientFrameworkExplanation_2.txt
      1 kB
      V.Narayanan
    4. ClientFrameworkExplanation_1.txt
      2 kB
      V.Narayanan
    5. lob_5.diff
      401 kB
      V.Narayanan
    6. lob_5.stat
      2 kB
      V.Narayanan
    7. lob_4.diff
      407 kB
      V.Narayanan
    8. lob_4.stat
      3 kB
      V.Narayanan
    9. ClientFramework_Explanation.txt
      3 kB
      V.Narayanan
    10. lob_3.diff
      407 kB
      V.Narayanan
    11. lob_2.diff
      412 kB
      V.Narayanan
    12. lob_1.diff
      413 kB
      V.Narayanan
    13. lob.diff
      381 kB
      V.Narayanan

      Activity

      Hide
      V.Narayanan added a comment -

      DETAILS OF PATCH

      Patch addresses the following issues

      1)Modifications to the ClientJDBCObjectFactory class and the incorporation of the factory into the Client layer.
      2)Blob and Clob method support

      Details of the above modifications are documented below.
      1)Modifications to ClientJDBCObjectFactory class

      a) Addition of method 'createObject' that uses reflection to create the objects depending on the version of VM under use and modifying ClientJDBCObjectFactory to use this method.

      b) Modifying Network Client classes to use the factory class.

      2)Blob and Clob method support

      The following are the methods from which the Util.notImplemented() exception has been removed and has been replaced with appropriate code.

      <trunk>/java/engine/org/apache/derby/impl/jdbc/EmbedPreparedStatement40.java

      setClob(int parameterIndex, Reader reader, long length)
      setBlob(int parameterIndex, InputStream inputStream, long length)

      <trunk>/java/client/org/apache/derby/client/am/PreparedStatement40.java

      setClob(int parameterIndex, Reader reader, long length)
      setBlob(int parameterIndex, InputStream inputStream, long length)

      Show
      V.Narayanan added a comment - DETAILS OF PATCH Patch addresses the following issues 1)Modifications to the ClientJDBCObjectFactory class and the incorporation of the factory into the Client layer. 2)Blob and Clob method support Details of the above modifications are documented below. 1)Modifications to ClientJDBCObjectFactory class a) Addition of method 'createObject' that uses reflection to create the objects depending on the version of VM under use and modifying ClientJDBCObjectFactory to use this method. b) Modifying Network Client classes to use the factory class. 2)Blob and Clob method support The following are the methods from which the Util.notImplemented() exception has been removed and has been replaced with appropriate code. <trunk>/java/engine/org/apache/derby/impl/jdbc/EmbedPreparedStatement40.java setClob(int parameterIndex, Reader reader, long length) setBlob(int parameterIndex, InputStream inputStream, long length) <trunk>/java/client/org/apache/derby/client/am/PreparedStatement40.java setClob(int parameterIndex, Reader reader, long length) setBlob(int parameterIndex, InputStream inputStream, long length)
      Hide
      Rick Hillegas added a comment -

      Hi Narayanan: The patch itself looks good. However, I'm seeing test failures:

      1) I'm seeing the following diff on lobStreams.java run under DerbyNetClient:

      MasterFileName = master/lobStreams.out
      1a2
      > extin\aclob.txt
      Test Failed.

          • End: lobStreams jdk1.4.2_08 DerbyNetClient 2006-01-05 07:09:44 ***

      2) All of the jdbc4 tests fail. I am running them according to the instructions in jira 587. Perhaps the instructions have changed? In a shell window using the 1.6 vm, I run the jdbc4 tests as follows:

      java -Dverbose=true org.apache.derbyTesting.functionTests.harness.RunSuite jdbc4

      and each test fails to get a connection:

      > Failed in creating a Connection: org.apache.derby.client.am.DisconnectException: java.security.PrivilegedActionException : Error opening socket to server xxxFILTERED_HOSTNAMExxx on port 1527 with message : null
      > org.apache.derby.client.am.DisconnectException: java.security.PrivilegedActionException : Error opening socket to server xxxFILTERED_HOSTNAMExxx on port 1527 with message : null

      Show
      Rick Hillegas added a comment - Hi Narayanan: The patch itself looks good. However, I'm seeing test failures: 1) I'm seeing the following diff on lobStreams.java run under DerbyNetClient: MasterFileName = master/lobStreams.out 1a2 > extin\aclob.txt Test Failed. End: lobStreams jdk1.4.2_08 DerbyNetClient 2006-01-05 07:09:44 *** 2) All of the jdbc4 tests fail. I am running them according to the instructions in jira 587. Perhaps the instructions have changed? In a shell window using the 1.6 vm, I run the jdbc4 tests as follows: java -Dverbose=true org.apache.derbyTesting.functionTests.harness.RunSuite jdbc4 and each test fails to get a connection: > Failed in creating a Connection: org.apache.derby.client.am.DisconnectException: java.security.PrivilegedActionException : Error opening socket to server xxxFILTERED_HOSTNAMExxx on port 1527 with message : null > org.apache.derby.client.am.DisconnectException: java.security.PrivilegedActionException : Error opening socket to server xxxFILTERED_HOSTNAMExxx on port 1527 with message : null
      Hide
      V.Narayanan added a comment -

      I have corrected the failures and am re-attaching the patch.

      Show
      V.Narayanan added a comment - I have corrected the failures and am re-attaching the patch.
      Hide
      Rick Hillegas added a comment -

      The code looks fine. Now derbyall passes cleanly. Also, the jdbc4 tests run cleanly on jdk1.6. I recommend that this patch be committed.

      Show
      Rick Hillegas added a comment - The code looks fine. Now derbyall passes cleanly. Also, the jdbc4 tests run cleanly on jdk1.6. I recommend that this patch be committed.
      Hide
      Daniel John Debrunner added a comment -

      The patch lacks comments, the methods that have changed from being not implemented have no java doc or comments. New methods added such as atLeast have no comments, the entire class ClientJDBCObjectFactory has no comments.

      I think EmbedPreparedStatement40.setClob()/setBlob() should be calling setCharacterStreamInternal()/setBinaryStreamInternal(). This would remove the need for reader/stream being null checks.

      I'm worried by this patch, or maybe previous patches in this series, reflection is used to allocate JDBC objects on the client, reflection is really slow, how often is this code invoked? E.g. is a NetResultSet allocated through reflection every time a ResultSet is returned to the client? The whole class ClientJDBCObjectFactory looks terrible for performance, creating any JDBC object now involves many object creations, which will slow down performance, even the the objects which could be static (the Class arrays) are created on very method call. Why was reflection used instead of a factory pattern like the embedded driver?

      I think at sometime we need some performance tests to ensure client performance has not regressed from 10.1

      Show
      Daniel John Debrunner added a comment - The patch lacks comments, the methods that have changed from being not implemented have no java doc or comments. New methods added such as atLeast have no comments, the entire class ClientJDBCObjectFactory has no comments. I think EmbedPreparedStatement40.setClob()/setBlob() should be calling setCharacterStreamInternal()/setBinaryStreamInternal(). This would remove the need for reader/stream being null checks. I'm worried by this patch, or maybe previous patches in this series, reflection is used to allocate JDBC objects on the client, reflection is really slow, how often is this code invoked? E.g. is a NetResultSet allocated through reflection every time a ResultSet is returned to the client? The whole class ClientJDBCObjectFactory looks terrible for performance, creating any JDBC object now involves many object creations, which will slow down performance, even the the objects which could be static (the Class arrays) are created on very method call. Why was reflection used instead of a factory pattern like the embedded driver? I think at sometime we need some performance tests to ensure client performance has not regressed from 10.1
      Hide
      Daniel John Debrunner added a comment -

      After looking a little more, currently setBinaryStreamInternal is not suitable, though it should be! Also looking at this I think I saw a bug in the exsiting setClob(int, Clob) method. There a long is converted to an int, and it is assumed that if the long is out of range that the result will be a negative integer. That is not true. It would probably be worth changing setCharacterStreamInternal and setBinaryStreamInternal to take a long for the length, and perform the > max int check in the internal methods. setBinaryStreamInternal could be shared if a valid length is passed in always, which would mean changing setBlob(int, Blob) to pass the length to setBinaryStreamInternal, as is done in setClob.

      Show
      Daniel John Debrunner added a comment - After looking a little more, currently setBinaryStreamInternal is not suitable, though it should be! Also looking at this I think I saw a bug in the exsiting setClob(int, Clob) method. There a long is converted to an int, and it is assumed that if the long is out of range that the result will be a negative integer. That is not true. It would probably be worth changing setCharacterStreamInternal and setBinaryStreamInternal to take a long for the length, and perform the > max int check in the internal methods. setBinaryStreamInternal could be shared if a valid length is passed in always, which would mean changing setBlob(int, Blob) to pass the length to setBinaryStreamInternal, as is done in setClob.
      Hide
      Sunitha Kambhampati added a comment -

      I actually have a patch to fix Derby-599, where I am changing the setBlob(int,Blob) to pass the length as is done in setClob to avoid materialization. I have some testing to do before I post the patch to the list (hopefully will get it out in a day or two). I can take care of changing the setBinaryStreamInternal to take a long for the length as part of the Derby 599 patch as it is related.

      Show
      Sunitha Kambhampati added a comment - I actually have a patch to fix Derby-599, where I am changing the setBlob(int,Blob) to pass the length as is done in setClob to avoid materialization. I have some testing to do before I post the patch to the list (hopefully will get it out in a day or two). I can take care of changing the setBinaryStreamInternal to take a long for the length as part of the Derby 599 patch as it is related.
      Hide
      V.Narayanan added a comment -

      Hi,

      Thank You for the comments Dan.

      a) Sorry about the comments and the javadocs. I will fix this in my next patch.

      b) I am asking Sunitha on how she is handling the length as a long datatype
      when the RawtoBinaryStream class expects an Integer.

      c) I have a solution where reflection would be used only once. I am
      documenting my solution below

      c.1) We could make ClientJDBCObjectFactory a interface
      c.2) Have two implementations ClientJDBCObjectFactoryImpl and
      ClientJDBCObjectFactoryImpl40
      c.3) A method getFactory() in the client driver that loads appropriate
      implementation depending on the jdk under use. This method would use
      reflection to load the JDBC40 class.

      This way we will have reflection only once.

      This could'nt be done exactly the same way as the embedded side because
      we do'nt have the monitor concept here

      Show
      V.Narayanan added a comment - Hi, Thank You for the comments Dan. a) Sorry about the comments and the javadocs. I will fix this in my next patch. b) I am asking Sunitha on how she is handling the length as a long datatype when the RawtoBinaryStream class expects an Integer. c) I have a solution where reflection would be used only once. I am documenting my solution below c.1) We could make ClientJDBCObjectFactory a interface c.2) Have two implementations ClientJDBCObjectFactoryImpl and ClientJDBCObjectFactoryImpl40 c.3) A method getFactory() in the client driver that loads appropriate implementation depending on the jdk under use. This method would use reflection to load the JDBC40 class. This way we will have reflection only once. This could'nt be done exactly the same way as the embedded side because we do'nt have the monitor concept here
      Hide
      V.Narayanan added a comment -

      As pointed out in the above comments reflection is used and this was making the process slow. This has been overcome with the following changes.

      1) ClientJDBCObjectFactory is an interface having two implementations

      a) ClientJDBCObjectFactoryImpl
      b) ClientJDBCObjectFactoryImpl40

      ClientJDBCObjectFactoryImpl is compiled by jdk1.4
      ClientJDBCObjectFactoryImpl40 is compiled by mustang (compiled optionally if jdk16 variable is present in ant.properties)

      2) The method getFactory() in the ClientDriver class would initialize a variable of type ClientJDBCObjectFactory with an appropriate implementation depending on the jdk version under use.

      3) This method is then used to return an instance of the factory implementation that will be used to return the approriate JDBC interface implementations to the user

      Issues that were addresses earlier and have been fixed
      ------------------------------------------------------

      1) comments have been added to the ClientJDBCObjectFactory,ClientJDBCObjectFactoryImpl,ClientJDBCObjectFactoryImpl40 classes. Comments have also been added to the atLeast method in Configuration.java

      2) I have raised a JIRA issue for setCharacterStreamInternal to be modified (DERBY-856) . setBinaryStreamInternal is being modified as part of DERBY-599

      3) Reflection would'nt be used for every object creation as in the previous case and would be only be used in getFactory to rerurn a factory implementation.

      thanx
      Narayanan

      Show
      V.Narayanan added a comment - As pointed out in the above comments reflection is used and this was making the process slow. This has been overcome with the following changes. 1) ClientJDBCObjectFactory is an interface having two implementations a) ClientJDBCObjectFactoryImpl b) ClientJDBCObjectFactoryImpl40 ClientJDBCObjectFactoryImpl is compiled by jdk1.4 ClientJDBCObjectFactoryImpl40 is compiled by mustang (compiled optionally if jdk16 variable is present in ant.properties) 2) The method getFactory() in the ClientDriver class would initialize a variable of type ClientJDBCObjectFactory with an appropriate implementation depending on the jdk version under use. 3) This method is then used to return an instance of the factory implementation that will be used to return the approriate JDBC interface implementations to the user Issues that were addresses earlier and have been fixed ------------------------------------------------------ 1) comments have been added to the ClientJDBCObjectFactory,ClientJDBCObjectFactoryImpl,ClientJDBCObjectFactoryImpl40 classes. Comments have also been added to the atLeast method in Configuration.java 2) I have raised a JIRA issue for setCharacterStreamInternal to be modified ( DERBY-856 ) . setBinaryStreamInternal is being modified as part of DERBY-599 3) Reflection would'nt be used for every object creation as in the previous case and would be only be used in getFactory to rerurn a factory implementation. thanx Narayanan
      Hide
      Daniel John Debrunner added a comment -

      One quick comment (I'm still reviewing the rest) in EmbedPreparedStatement40.

      The code throwing exceptions seems strange to me, and I don't think matches code elsewhere.

      + throw Util.newEmbedSQLException(SQLState.LANG_INVALID_COLUMN_LENGTH,
      + new Object[]

      {new String("setBlob()")}

      ,
      + StandardException.getSeverityFromIdentifier
      + (SQLState.LANG_INVALID_COLUMN_LENGTH));

      1) I don't understand the code new String("setBlob()") - "setBlob()" is already a String, why convert it to a String again?
      In fact I never understood why that constructor String(String s) exists!

      2) Usually when throwing exceptions, the calling code doesn't create the Object array for the arguments, or calculate the severity. It's handled by utility methods. Look at other examples in EmbedPreparedStatement, e.g.

      line 1239
      throw newSQLException(SQLState.NO_INPUT_PARAMETERS)

      line 1247
      throw newSQLException(SQLState.LANG_INVALID_PARAM_POSITION,
      new Integer(parameterIndex), new Integer(types.length))

      Show
      Daniel John Debrunner added a comment - One quick comment (I'm still reviewing the rest) in EmbedPreparedStatement40. The code throwing exceptions seems strange to me, and I don't think matches code elsewhere. + throw Util.newEmbedSQLException(SQLState.LANG_INVALID_COLUMN_LENGTH, + new Object[] {new String("setBlob()")} , + StandardException.getSeverityFromIdentifier + (SQLState.LANG_INVALID_COLUMN_LENGTH)); 1) I don't understand the code new String("setBlob()") - "setBlob()" is already a String, why convert it to a String again? In fact I never understood why that constructor String(String s) exists! 2) Usually when throwing exceptions, the calling code doesn't create the Object array for the arguments, or calculate the severity. It's handled by utility methods. Look at other examples in EmbedPreparedStatement, e.g. line 1239 throw newSQLException(SQLState.NO_INPUT_PARAMETERS) line 1247 throw newSQLException(SQLState.LANG_INVALID_PARAM_POSITION, new Integer(parameterIndex), new Integer(types.length))
      Hide
      V.Narayanan added a comment -

      Thanx for the comments

      You are right it should be "SetBlob()" only. I will fix this.

      I was confused with the usage of the newEmbedSQLException methods in Util.java. For example when throwing the notImplemented Exception the following is done

      return newEmbedSQLException(SQLState.NOT_IMPLEMENTED,
      new Object[]

      {MessageService.getTextMessage(MessageId.CONN_NO_DETAILS)}

      ,
      StandardException.getSeverityFromIdentifier(SQLState.NOT_IMPLEMENTED));

      but as you say that is an utility method. Thank you for pointing this out. I will change this to match the signature in EmbedPreparedStatement. I will submit a new patch with this change incorporated.

      thanx once again

      Narayanan

      Show
      V.Narayanan added a comment - Thanx for the comments You are right it should be "SetBlob()" only. I will fix this. I was confused with the usage of the newEmbedSQLException methods in Util.java. For example when throwing the notImplemented Exception the following is done return newEmbedSQLException(SQLState.NOT_IMPLEMENTED, new Object[] {MessageService.getTextMessage(MessageId.CONN_NO_DETAILS)} , StandardException.getSeverityFromIdentifier(SQLState.NOT_IMPLEMENTED)); but as you say that is an utility method. Thank you for pointing this out. I will change this to match the signature in EmbedPreparedStatement. I will submit a new patch with this change incorporated. thanx once again Narayanan
      Hide
      V.Narayanan added a comment -

      I have changed it to work in the same pattern as in EmbedPreparedStatement (i.e.) it does a
      throw newSQLException(SQLState.LANG_INVALID_COLUMN_LENGTH);
      thanx
      Narayanan

      Show
      V.Narayanan added a comment - I have changed it to work in the same pattern as in EmbedPreparedStatement (i.e.) it does a throw newSQLException(SQLState.LANG_INVALID_COLUMN_LENGTH); thanx Narayanan
      Hide
      V.Narayanan added a comment -

      Hi Dan,
      I have changed the patch to use newSQLException. I will also put the patch in for setCharacterStreamInternal with the suggessted changes. Can you please tell me any other changes that would be required for this patch so that I can do the same and submit it again.
      thanx,
      Narayanan

      Show
      V.Narayanan added a comment - Hi Dan, I have changed the patch to use newSQLException. I will also put the patch in for setCharacterStreamInternal with the suggessted changes. Can you please tell me any other changes that would be required for this patch so that I can do the same and submit it again. thanx, Narayanan
      Hide
      V.Narayanan added a comment -

      Hi Dan,
      Sorry if this request is redundant but can you please tell me if this patch is good or if you think there are issues that need to be addressed so that I can do the same in a subsequent patch.
      thanx
      Narayanan

      Show
      V.Narayanan added a comment - Hi Dan, Sorry if this request is redundant but can you please tell me if this patch is good or if you think there are issues that need to be addressed so that I can do the same in a subsequent patch. thanx Narayanan
      Hide
      Daniel John Debrunner added a comment -

      I thought the patch was going to be re-written to use the setBinaryStreamInternal and setCharacterStreamInternal that have been modified to take a long length argument.

      I think someone more familiar with the client side should review those changes, the more eyes the better.

      Show
      Daniel John Debrunner added a comment - I thought the patch was going to be re-written to use the setBinaryStreamInternal and setCharacterStreamInternal that have been modified to take a long length argument. I think someone more familiar with the client side should review those changes, the more eyes the better.
      Hide
      V.Narayanan added a comment -

      1) lob_3.diff is the new patch being attached
      2) I have modified setClob to use setCharacterStreamInternal and setBlob to use setBinaryStreamInternal.

      Narayanan

      Show
      V.Narayanan added a comment - 1) lob_3.diff is the new patch being attached 2) I have modified setClob to use setCharacterStreamInternal and setBlob to use setBinaryStreamInternal. Narayanan
      Hide
      V.Narayanan added a comment -

      description of the changes made in this patch

      Show
      V.Narayanan added a comment - description of the changes made in this patch
      Hide
      Rick Hillegas added a comment -

      I'm a bit muddled and it might be that JIRA is misbehaving again. When I downloaded lob_3.diff, it didn't contain ClientJDBCObjectFactory and its implementations. Have those changes been moved out to another patch?

      Show
      Rick Hillegas added a comment - I'm a bit muddled and it might be that JIRA is misbehaving again. When I downloaded lob_3.diff, it didn't contain ClientJDBCObjectFactory and its implementations. Have those changes been moved out to another patch?
      Hide
      V.Narayanan added a comment -

      I hadnt moved the changes to another patch. I tried downloading it today, it does contain the changes. I will anyway attach it once again and also attach the status file.

      Show
      V.Narayanan added a comment - I hadnt moved the changes to another patch. I tried downloading it today, it does contain the changes. I will anyway attach it once again and also attach the status file.
      Hide
      V.Narayanan added a comment -

      Attaching it again

      Show
      V.Narayanan added a comment - Attaching it again
      Hide
      Rick Hillegas added a comment -

      I have downloaded lob_4.diff. It appears to satisfy the previous objections: 1) reflection is no longer used to create network client jdbc objects, 2) calls to the streaming minions use long stream lengths, 3) SQLExceptions are created in the regular way.

      The patch builds on my machine and passes the jdbc4 suite. I will run derbyall once the regressions in the trunk settle down.

      I am very keen to commit this patch because several other chunks of JDBC4 work depend on it. However, I'm also eager for this patch to be reviewed by one of our network experts (Kathey? Bryan?). Hopefully, after all this time, the comments will be fit and polish issues which Narayanan can address in a follow-on submission.

      Thanks in advance for your kind attention to this urgent patch.

      Show
      Rick Hillegas added a comment - I have downloaded lob_4.diff. It appears to satisfy the previous objections: 1) reflection is no longer used to create network client jdbc objects, 2) calls to the streaming minions use long stream lengths, 3) SQLExceptions are created in the regular way. The patch builds on my machine and passes the jdbc4 suite. I will run derbyall once the regressions in the trunk settle down. I am very keen to commit this patch because several other chunks of JDBC4 work depend on it. However, I'm also eager for this patch to be reviewed by one of our network experts (Kathey? Bryan?). Hopefully, after all this time, the comments will be fit and polish issues which Narayanan can address in a follow-on submission. Thanks in advance for your kind attention to this urgent patch.
      Hide
      Daniel John Debrunner added a comment -

      The embedded changes look good.

      Is there any reason for the changes in the TestConnection.java class, such as database name from mydb to mydb1?
      And is there any good reason for this class, why don't these tests get the connections the standard way, used by all
      other tests in the harness?

      I see this pattern in the tests for failed exceptions:

      + catch(IOException ioe)

      { + System.out.println("Input output exception occurred" + ioe); + }

      + catch(SQLException sqle)

      { + System.out.println("SQLException occurred" + sqle); + }

      I guess it's not critical but this approach to displaying errors is a real pain when you actually hit a failure.

      Just printing what you print means that on a failure, I have to find which line was causing the error,
      not easy if one uses the same text like "SQLException occurred", modify that code to print the stack trace,
      modify the code to print the SQLException chain, and then re-run the test.

      Printing the stack trace, and for SQLExceptions the complete nested chain and their stack traces can often
      mean spotting what the problem is instantly.

      Show
      Daniel John Debrunner added a comment - The embedded changes look good. Is there any reason for the changes in the TestConnection.java class, such as database name from mydb to mydb1? And is there any good reason for this class, why don't these tests get the connections the standard way, used by all other tests in the harness? I see this pattern in the tests for failed exceptions: + catch(IOException ioe) { + System.out.println("Input output exception occurred" + ioe); + } + catch(SQLException sqle) { + System.out.println("SQLException occurred" + sqle); + } I guess it's not critical but this approach to displaying errors is a real pain when you actually hit a failure. Just printing what you print means that on a failure, I have to find which line was causing the error, not easy if one uses the same text like "SQLException occurred", modify that code to print the stack trace, modify the code to print the SQLException chain, and then re-run the test. Printing the stack trace, and for SQLExceptions the complete nested chain and their stack traces can often mean spotting what the problem is instantly.
      Hide
      V.Narayanan added a comment -

      Thanx a ton for the reviews and comments.

      I have tried to answer the queries raised for this issue.

      Issues Raised and the explanations follow
      ----------------------------------------------------------------------------

      Issue 1
      -------------

      Change of name of the database from mydb to mydb1
      -------------------------------------------------------------------------------------------------

      Since I was creating the databases from my code I wanted to give meaningful names to the databases that I create to test the Embedded and Network JDBC4 code changes. I initially changed it to mydb1 to check for the run with different databases but missed out on changing the actual names. I apologize for this mistake I will change this to a more meaningful name say "jdbc4test_database".

      Issue 2
      -------------

      Printing the stack trace
      -----------------------------------------

      I will do this change and re-submit my patch

      Issue 3
      -------------

      Why was the TestConnection class created?
      ---------------------------------------------------------------------------------

      This class was created during the initial days when I wrote the JDBC4 suite. I understand that I can do a ij.startJBMS() to get a connection. I notice that doing a ij.getJBMS() from inside a 1.6 compiled class returns null with no message. Thanx for pointing this David. I will remove this class and change it the usual way once I fix the ij.startJBMS() bug. I will raise a JIRA issue for this.

      Issue 4
      -------------

      • if (offset_ > (blob_.binaryString_.length - blob_.dataOffset_)) {
        + if ((offset_-1) > (blob_.binaryString_.length - blob_.dataOffset_)) {
        -------------------------------------------------------------------------------------------------------------------------------

      Sorry for having missed this comment I will do this.

      Issue 5
      -------------

      — java/client/org/apache/derby/client/am/Blob.java (revision 377622)
      +++ java/client/org/apache/derby/client/am/Blob.java (working copy)
      @@ -267,7 +267,10 @@

      public int setBytesX(long pos, byte[] bytes, int offset, int len) throws SqlException {
      int length = 0;

      • if ((int) pos <= 0) {
        + boolean insertIntoEmpty = false;
        + if(pos==1 && binaryString_.length==0)
        + insertIntoEmpty = true;
        + if ((int) pos <= 0 || ((!insertIntoEmpty) && (pos > binaryString_.length - dataOffset_))) { throw new SqlException(agent_.logWriter_, new MessageId(SQLState.BLOB_BAD_POSITION), new Long(pos)); }

      It seemed like the essence of this change was something like:

      "It's usually an error to pass a 'pos' value greater than the
      length of the blob, but if the blob is currently empty then
      there is a special case where the caller passes 1, not 0, as
      you might expect."

      The interpretation above is exactly what I tried to do. I will try to explain what I intended and also restructure the code to make it more understandable.

      There are two cases when exception needs to be thrown

      a) when pos <=0
      b) when pos > binaryString_.length - dataOffset_
      b.1) This case arises when your insert into a empty Blob. so here pos = 1 and (binaryString_.length - dataOffset_)=0.
      this should not result in a SQL exception being thrown.

      I will split this into two parts by doing this
      if(pos<=0)
      Throw an exception
      if(pos > binaryString_.length - dataOffset_)
      {
      if(Insert in not into an empty Blob)

      { Throw an exception }

      }

      Issue - 6
      -----------------

      it is a cleaner approach to do the conversion from one-based index to zero-based index on the highest possible level
      -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

      Would it be acceptable if I add a proper comment explaining my change for now and raise this as a seperate issue and fix it at the earliest?

      issue - 7
      -----------------

      ij.startJBMS() throws error
      -----------------------------------------------

      Thank you for pointing this out. I will raise an issue for this one

      issue - 8
      ----------------

      Appearence of unwanted changes as part of diff
      --------------------------------------------------------------------------------------

      eg


      • public void setSQLXML(String parameterName, SQLXML xmlObject) throws SQLException {
        + public void setSQLXML(String parameterName, SQLXML xmlObject)
        + throws SQLException {

      I had tried to change the diff to keep to 80 characters per line wherever I noticed a deviation. The original was 83. I wanted to split it to keep it to 80. If it is felt that this is a distraction I will change it to the original and submit another patch to make this change.

      issue-9
      -------------

      Appearence of the variable con in ClientDataSource.java

      I will remove this.

      Thanx once again for the reviews and comments.

      Narayanan

      Show
      V.Narayanan added a comment - Thanx a ton for the reviews and comments. I have tried to answer the queries raised for this issue. Issues Raised and the explanations follow ---------------------------------------------------------------------------- Issue 1 ------------- Change of name of the database from mydb to mydb1 ------------------------------------------------------------------------------------------------- Since I was creating the databases from my code I wanted to give meaningful names to the databases that I create to test the Embedded and Network JDBC4 code changes. I initially changed it to mydb1 to check for the run with different databases but missed out on changing the actual names. I apologize for this mistake I will change this to a more meaningful name say "jdbc4test_database". Issue 2 ------------- Printing the stack trace ----------------------------------------- I will do this change and re-submit my patch Issue 3 ------------- Why was the TestConnection class created? --------------------------------------------------------------------------------- This class was created during the initial days when I wrote the JDBC4 suite. I understand that I can do a ij.startJBMS() to get a connection. I notice that doing a ij.getJBMS() from inside a 1.6 compiled class returns null with no message. Thanx for pointing this David. I will remove this class and change it the usual way once I fix the ij.startJBMS() bug. I will raise a JIRA issue for this. Issue 4 ------------- if (offset_ > (blob_.binaryString_.length - blob_.dataOffset_)) { + if ((offset_-1) > (blob_.binaryString_.length - blob_.dataOffset_)) { ------------------------------------------------------------------------------------------------------------------------------- Sorry for having missed this comment I will do this. Issue 5 ------------- — java/client/org/apache/derby/client/am/Blob.java (revision 377622) +++ java/client/org/apache/derby/client/am/Blob.java (working copy) @@ -267,7 +267,10 @@ public int setBytesX(long pos, byte[] bytes, int offset, int len) throws SqlException { int length = 0; if ((int) pos <= 0) { + boolean insertIntoEmpty = false; + if(pos==1 && binaryString_.length==0) + insertIntoEmpty = true; + if ((int) pos <= 0 || ((!insertIntoEmpty) && (pos > binaryString_.length - dataOffset_))) { throw new SqlException(agent_.logWriter_, new MessageId(SQLState.BLOB_BAD_POSITION), new Long(pos)); } It seemed like the essence of this change was something like: "It's usually an error to pass a 'pos' value greater than the length of the blob, but if the blob is currently empty then there is a special case where the caller passes 1, not 0, as you might expect." The interpretation above is exactly what I tried to do. I will try to explain what I intended and also restructure the code to make it more understandable. There are two cases when exception needs to be thrown a) when pos <=0 b) when pos > binaryString_.length - dataOffset_ b.1) This case arises when your insert into a empty Blob. so here pos = 1 and (binaryString_.length - dataOffset_)=0. this should not result in a SQL exception being thrown. I will split this into two parts by doing this if(pos<=0) Throw an exception if(pos > binaryString_.length - dataOffset_) { if(Insert in not into an empty Blob) { Throw an exception } } Issue - 6 ----------------- it is a cleaner approach to do the conversion from one-based index to zero-based index on the highest possible level ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Would it be acceptable if I add a proper comment explaining my change for now and raise this as a seperate issue and fix it at the earliest? issue - 7 ----------------- ij.startJBMS() throws error ----------------------------------------------- Thank you for pointing this out. I will raise an issue for this one issue - 8 ---------------- Appearence of unwanted changes as part of diff -------------------------------------------------------------------------------------- eg public void setSQLXML(String parameterName, SQLXML xmlObject) throws SQLException { + public void setSQLXML(String parameterName, SQLXML xmlObject) + throws SQLException { I had tried to change the diff to keep to 80 characters per line wherever I noticed a deviation. The original was 83. I wanted to split it to keep it to 80. If it is felt that this is a distraction I will change it to the original and submit another patch to make this change. issue-9 ------------- Appearence of the variable con in ClientDataSource.java I will remove this. Thanx once again for the reviews and comments. Narayanan
      Hide
      V.Narayanan added a comment -

      I have explained the changes made in the attachment ClientFrameworkExplanation_1.txt

      Show
      V.Narayanan added a comment - I have explained the changes made in the attachment ClientFrameworkExplanation_1.txt
      Hide
      Rick Hillegas added a comment -

      The test runs look good: The jdbc4 suite runs cleanly against the lob_5.diff changes. Derbyall suffered two failures (wisconsin and xaSimplePositive); however, we see these same failures in Ole's automated tests.

      Show
      Rick Hillegas added a comment - The test runs look good: The jdbc4 suite runs cleanly against the lob_5.diff changes. Derbyall suffered two failures (wisconsin and xaSimplePositive); however, we see these same failures in Ole's automated tests.
      Hide
      V.Narayanan added a comment -

      Thanx a ton for the detailed reviews and comments on the patch. I am working to get the issues with this patch resolved. I am summarizing the issues reported as part of the reviews as a series of points below. I will fix them and resubmit the patch

      Issue-1
      -------
      Javadoc needs to have @param/@return tags

      Issue-2
      -------

      Some classes/methods do not have javadoc/comments

      specifc case

      ClientJDBCObjectfactory
      ClientJDBCObjectFactoryImpl
      ClientJDBCObjectFactoryImpl40

      --> In this specific case I did'nt put in method level javadoc comments for the Impl classes because they inherit the javadoc comments of the interface. If you still feel that they need class level comments I will add these too.

      Issue-3
      -------
      The indentation style is not consistent and makes the code harder
      to read.

      Issue-4
      -------
      In ClientConnectionPoolDataSource I don't see any reason to catch the SQLException since it's just going
      to be thrown again anyway.

      Issue-5
      -------
      I am combining all the issues related to createDefaultFactoryImpl() here

      createDefaultFactoryImpl() is called in the exception handler, but
      the return value is discarded. I guess it is supposed to be returned.

      The way exceptions are handled (in particular the use
      of instanceof and throwing away the stack trace) is improper.

      don't see much value in the createObject() method, since it's
      just calling Class.forName(...).newInstance() and is just being
      used once

      Issue-6
      -------

      The method Configuration.atLeast() duplicates functionality found in
      JVMInfo.

      Preferably, you could create a method supportsJDBC40() that
      returns a boolean.

      Issue-7
      -------

      moving ClientJDBCObjectFactoryImpl and ClientJDBCObjectFactoryImpl40 to net
      package

      renaming method signatures inside this to be more generic

      thanx once again
      Narayanan

      Show
      V.Narayanan added a comment - Thanx a ton for the detailed reviews and comments on the patch. I am working to get the issues with this patch resolved. I am summarizing the issues reported as part of the reviews as a series of points below. I will fix them and resubmit the patch Issue-1 ------- Javadoc needs to have @param/@return tags Issue-2 ------- Some classes/methods do not have javadoc/comments specifc case ClientJDBCObjectfactory ClientJDBCObjectFactoryImpl ClientJDBCObjectFactoryImpl40 --> In this specific case I did'nt put in method level javadoc comments for the Impl classes because they inherit the javadoc comments of the interface. If you still feel that they need class level comments I will add these too. Issue-3 ------- The indentation style is not consistent and makes the code harder to read. Issue-4 ------- In ClientConnectionPoolDataSource I don't see any reason to catch the SQLException since it's just going to be thrown again anyway. Issue-5 ------- I am combining all the issues related to createDefaultFactoryImpl() here createDefaultFactoryImpl() is called in the exception handler, but the return value is discarded. I guess it is supposed to be returned. The way exceptions are handled (in particular the use of instanceof and throwing away the stack trace) is improper. don't see much value in the createObject() method, since it's just calling Class.forName(...).newInstance() and is just being used once Issue-6 ------- The method Configuration.atLeast() duplicates functionality found in JVMInfo. Preferably, you could create a method supportsJDBC40() that returns a boolean. Issue-7 ------- moving ClientJDBCObjectFactoryImpl and ClientJDBCObjectFactoryImpl40 to net package renaming method signatures inside this to be more generic thanx once again Narayanan
      Hide
      V.Narayanan added a comment -

      Please find attached the patch with the issues addressed
      Narayanan

      Show
      V.Narayanan added a comment - Please find attached the patch with the issues addressed Narayanan
      Hide
      Knut Anders Hatlen added a comment -

      Thanks for the improved patch, Narayanan! You have addressed most of
      my concerns, but there is still one issue:

      ClientJDBCObjectFactory (in the am package) has a lot of references to
      classes in the net package. There should not be any dependencies from
      am to net.

      And while you're at it, there's a white-space change in
      am/PreparedStatement.java that wasn't in your previous patch:

      public class PreparedStatement extends Statement
      implements java.sql.PreparedStatement,

      • PreparedStatementCallbackInterface {
        + PreparedStatementCallbackInterface{
      Show
      Knut Anders Hatlen added a comment - Thanks for the improved patch, Narayanan! You have addressed most of my concerns, but there is still one issue: ClientJDBCObjectFactory (in the am package) has a lot of references to classes in the net package. There should not be any dependencies from am to net. And while you're at it, there's a white-space change in am/PreparedStatement.java that wasn't in your previous patch: public class PreparedStatement extends Statement implements java.sql.PreparedStatement, PreparedStatementCallbackInterface { + PreparedStatementCallbackInterface{
      Hide
      V.Narayanan added a comment -

      Fixed the issues pointed out by knut in the earlier comments and am posting the patch again. In the current patch I am handling the issue pointed out by knut of the ClientJDBCObjectFatory class not to have any reference of net package.

      However because of this change in ClientJDBCObjectFactory please observe the following cast of NetLogWriter in ClientJDBCObjectFactoryImpl

      Line no 134

      return (org.apache.derby.client.am.Connection)
      (new NetConnection((NetLogWriter)netLogWriter,user,password,dataSource,rmId,
      isXAConn));

      This cast won't fail since newConnection method is being called internally and is always called with NetLogWriter. However a better solution would be to change the NetConnection class constructors to accept LogWriter.

      I am opening a new JIRA issue for this and along with that patch I will submit the changes to remove the explicit casts in ClientJDBCObjectFactoryImpl and ClientJDBCObjectFactoryImpl40

      Narayanan

      Show
      V.Narayanan added a comment - Fixed the issues pointed out by knut in the earlier comments and am posting the patch again. In the current patch I am handling the issue pointed out by knut of the ClientJDBCObjectFatory class not to have any reference of net package. However because of this change in ClientJDBCObjectFactory please observe the following cast of NetLogWriter in ClientJDBCObjectFactoryImpl Line no 134 return (org.apache.derby.client.am.Connection) (new NetConnection((NetLogWriter)netLogWriter,user,password,dataSource,rmId, isXAConn)); This cast won't fail since newConnection method is being called internally and is always called with NetLogWriter. However a better solution would be to change the NetConnection class constructors to accept LogWriter. I am opening a new JIRA issue for this and along with that patch I will submit the changes to remove the explicit casts in ClientJDBCObjectFactoryImpl and ClientJDBCObjectFactoryImpl40 Narayanan
      Hide
      Knut Anders Hatlen added a comment -

      Thanks again for fixing up the patch, Narayanan. I have no further
      comments to the client code. Great work!

      I haven't actually tested the patch with Mustang and JDBC 4.0, but
      derbyall passes on Sun JVM 1.4.2/Solaris 10 x86.

      Show
      Knut Anders Hatlen added a comment - Thanks again for fixing up the patch, Narayanan. I have no further comments to the client code. Great work! I haven't actually tested the patch with Mustang and JDBC 4.0, but derbyall passes on Sun JVM 1.4.2/Solaris 10 x86.
      Hide
      Rick Hillegas added a comment -

      The jdb4 tests run cleanly under 1.6. Derbyall ran cleanly for me on cygwin/xp under 1.4, modulo pre-existing failures in wisconsin, syscat, and xaSimplePositive. I have committed this patch with subversion rev 380187.

      Show
      Rick Hillegas added a comment - The jdb4 tests run cleanly under 1.6. Derbyall ran cleanly for me on cygwin/xp under 1.4, modulo pre-existing failures in wisconsin, syscat, and xaSimplePositive. I have committed this patch with subversion rev 380187.
      Hide
      Andrew McIntyre added a comment -

      This issue has been resolved for over a year with no further movement. Closing.

      Show
      Andrew McIntyre added a comment - This issue has been resolved for over a year with no further movement. Closing.

        People

        • Assignee:
          V.Narayanan
          Reporter:
          V.Narayanan
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development