Derby
  1. Derby
  2. DERBY-4715

Write jvm information and path of derby.jar to derby.log

    Details

    • Issue & fix info:
      Newcomer, Patch Available
    • Bug behavior facts:
      Seen in production

      Description

      The bug is part of DERBY-1272. In production environment, derby.jar can be located different than the derbyclient.jar It can be easier if we have jvm version information and path of derby.jar are in the derby.log

      1. derby.log
        1 kB
        Lily Wei
      2. DERBY-4715-1.diff
        6 kB
        Lily Wei
      3. DERBY-4715-10.5_967304.diff
        20 kB
        Lily Wei
      4. DERBY-4715-10.6_967304.diff
        21 kB
        Lily Wei
      5. DERBY-4715-2.diff
        19 kB
        Lily Wei
      6. DERBY-4715-3.diff
        19 kB
        Lily Wei
      7. DERBY-4715-4.diff
        19 kB
        Lily Wei
      8. DERBY-4715-5.diff
        19 kB
        Lily Wei
      9. DERBY-4715-6.diff
        3 kB
        Lily Wei
      10. DERBY-4715-7.diff
        3 kB
        Lily Wei
      11. DERBY-4715-8.diff
        3 kB
        Lily Wei

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          28d 8h 32m 1 Lily Wei 24/Jul/10 01:19
          Resolved Resolved Closed Closed
          12d 13h 47m 1 Lily Wei 05/Aug/10 15:07
          Gavin made changes -
          Workflow jira [ 12514319 ] Default workflow, editable Closed status [ 12799744 ]
          Kathey Marsden made changes -
          Link This issue relates to DERBY-4944 [ DERBY-4944 ]
          Rick Hillegas made changes -
          Fix Version/s 10.7.1.1 [ 12315564 ]
          Fix Version/s 10.7.1.0 [ 12314971 ]
          Knut Anders Hatlen made changes -
          Fix Version/s 10.6.2.1 [ 12315343 ]
          Fix Version/s 10.6.2.0 [ 12315342 ]
          Kathey Marsden made changes -
          Fix Version/s 10.6.2.0 [ 12315342 ]
          Fix Version/s 10.6.1.1 [ 12314973 ]
          Lily Wei made changes -
          Fix Version/s 10.5.3.1 [ 12314182 ]
          Fix Version/s 10.6.1.1 [ 12314973 ]
          Hide
          Lily Wei added a comment -

          Add fix version.

          Show
          Lily Wei added a comment - Add fix version.
          Lily Wei made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Bug behavior facts [Seen in production]
          Hide
          Lily Wei added a comment -

          Thanks, Kristian.

          It makes my day to know that Hudson does not find any problems with my backport. I finally know the build details on Hudson. That is so good to know.

          Myrna also told me about svn merge -c REV tip. I was using that do to this merge. I will close this JIRA. It is more belong to additional information is needed category.

          Show
          Lily Wei added a comment - Thanks, Kristian. It makes my day to know that Hudson does not find any problems with my backport. I finally know the build details on Hudson. That is so good to know. Myrna also told me about svn merge -c REV tip. I was using that do to this merge. I will close this JIRA. It is more belong to additional information is needed category.
          Hide
          Kristian Waagan added a comment -

          Thanks, Lily.

          You only get an email from Hudson if something goes wrong with the build. As you can see here [1], it didn't find any problems with your backport to 10.6.
          The job is checking the repos hourly, and the job itself is quickly done, so if you introduce a build break (or a JavaDoc warning) you should get an email within one and a half hour or so.

          Another tip when merging a single revision is to use svn merge -c REV instead of svn merge -r REV-1:REV.

          [1] http://hudson.zones.apache.org/hudson/job/Derby-branch-10.6/23/

          Show
          Kristian Waagan added a comment - Thanks, Lily. You only get an email from Hudson if something goes wrong with the build. As you can see here [1] , it didn't find any problems with your backport to 10.6. The job is checking the repos hourly, and the job itself is quickly done, so if you introduce a build break (or a JavaDoc warning) you should get an email within one and a half hour or so. Another tip when merging a single revision is to use svn merge -c REV instead of svn merge -r REV-1:REV. [1] http://hudson.zones.apache.org/hudson/job/Derby-branch-10.6/23/
          Hide
          Lily Wei added a comment -

          Thanks Kristian for reviewing the merge code. After 'ant clobber all buildjars' and 'ant javadoc', suites.All and derbyall test suites are passing. I did some tests against ibm jvm for this merge. I commit the merge to 10.5 and 10.6 branches.

          I hope Hudson build will pick up the changelist. I did not get emails after commit my code. I hope not receiving emails will not delay me to react to any issue cause by my commit. However, I know you guys are always watching too. Please let me know if there is anything I should pay attention to.

          The wiki-page is very helpful. Thank you. I shall add any new information as I discover them. The new svn is 982370(10.6) and 982401(10.5).

          Show
          Lily Wei added a comment - Thanks Kristian for reviewing the merge code. After 'ant clobber all buildjars' and 'ant javadoc', suites.All and derbyall test suites are passing. I did some tests against ibm jvm for this merge. I commit the merge to 10.5 and 10.6 branches. I hope Hudson build will pick up the changelist. I did not get emails after commit my code. I hope not receiving emails will not delay me to react to any issue cause by my commit. However, I know you guys are always watching too. Please let me know if there is anything I should pay attention to. The wiki-page is very helpful. Thank you. I shall add any new information as I discover them. The new svn is 982370(10.6) and 982401(10.5).
          Hide
          Kristian Waagan added a comment -

          If the fix merged cleanly (svn merge succeeded), and you were able to do a clean build and ran the tests without (new) errors, you can basically commit.
          I prefer to do a clean build to make sure all changes are picked up; I run 'ant clobber all buildjars'. In some rare cases you can also introduce JavaDoc errors by doing a backport, so it doesn't hurt to run the JavaDoc target either. For the newer branches this will be picked up by the Hudson build jobs, and a mail will be sent to derby-dev (plus the committer(s) who introduced the change(s)).

          You may have seen this already, but there are some guidelines at http://wiki.apache.org/db-derby/MoveAChangeBetweenBranches
          From a quick glance, it seems you can save yourself some time by not posting patches if the merge is clean
          The wiki-page seems to be rather up to date, but I think there are a few issues there that would benefit from a refresh.

          Show
          Kristian Waagan added a comment - If the fix merged cleanly (svn merge succeeded), and you were able to do a clean build and ran the tests without (new) errors, you can basically commit. I prefer to do a clean build to make sure all changes are picked up; I run 'ant clobber all buildjars'. In some rare cases you can also introduce JavaDoc errors by doing a backport, so it doesn't hurt to run the JavaDoc target either. For the newer branches this will be picked up by the Hudson build jobs, and a mail will be sent to derby-dev (plus the committer(s) who introduced the change(s)). You may have seen this already, but there are some guidelines at http://wiki.apache.org/db-derby/MoveAChangeBetweenBranches From a quick glance, it seems you can save yourself some time by not posting patches if the merge is clean The wiki-page seems to be rather up to date, but I think there are a few issues there that would benefit from a refresh.
          Lily Wei made changes -
          Attachment DERBY-4715-10.5_967304.diff [ 12451126 ]
          Attachment DERBY-4715-10.6_967304.diff [ 12451127 ]
          Hide
          Lily Wei added a comment -

          I back port the fix for two changelist 965647 and 967304 to 10.5 and 10.6 port. Attach the patch files. Please review it. Both pass suites.all and derbyall test suites on 10.5 and 10.6.

          Show
          Lily Wei added a comment - I back port the fix for two changelist 965647 and 967304 to 10.5 and 10.6 port. Attach the patch files. Please review it. Both pass suites.all and derbyall test suites on 10.5 and 10.6.
          Hide
          Kristian Waagan added a comment -

          Yes, I agree that we should not change the indentation for lines we don't touch in a patch. My point was about lines added or changed as part of a patch.
          Due to better tools this mixing of tabs and spaces isn't such a big deal any more, but still the tools mess up at times (for instance changing the indentation on lines not changed by the fix itself, or when viewing diffs/code in a terminal).

          For the curious, here's where we stand today:

          Whitespace characteristics (r967304)
          ==================================

          • Files
            Spaces only : 1056 37.34%
            Tabs only : 0 0.00%
            Mixed : 1772 62.66%
          • Lines
            Spaces only : 597974 52.23%
            Tabs only : 281128 24.55%
            Mixed : 87550 7.65%
            Blank : 131378 11.47%
            No indent : 46943 4.00%

          We can see that even changing just the lines which mix both tabs and spaces would cause a huge amount of noise (and, as you say, make digging in the Derby history more difficult), which is why I'm most comfortable with just letting the situation improve gradually. Then, in a rather distant future...

          Show
          Kristian Waagan added a comment - Yes, I agree that we should not change the indentation for lines we don't touch in a patch. My point was about lines added or changed as part of a patch. Due to better tools this mixing of tabs and spaces isn't such a big deal any more, but still the tools mess up at times (for instance changing the indentation on lines not changed by the fix itself, or when viewing diffs/code in a terminal). For the curious, here's where we stand today: Whitespace characteristics (r967304) ================================== Files Spaces only : 1056 37.34% Tabs only : 0 0.00% Mixed : 1772 62.66% Lines Spaces only : 597974 52.23% Tabs only : 281128 24.55% Mixed : 87550 7.65% Blank : 131378 11.47% No indent : 46943 4.00% We can see that even changing just the lines which mix both tabs and spaces would cause a huge amount of noise (and, as you say, make digging in the Derby history more difficult), which is why I'm most comfortable with just letting the situation improve gradually. Then, in a rather distant future...
          Hide
          Lily Wei added a comment -

          Thank you for reviewing the patch, Kristian. I will prefer using spaces for indentation as well.
          However, for file BaseDataFileFactory.java, there are way too many lines changes if I save spaces as tab for indentation using editor/IDE to insert spaces when I press the TAB key. That is why both spaces and tab were using for the patch. I think it is also important to keep history as we introduce insert spaces as tab for indentation going forward. We could have a project that only changes tab to spaces.

          Show
          Lily Wei added a comment - Thank you for reviewing the patch, Kristian. I will prefer using spaces for indentation as well. However, for file BaseDataFileFactory.java, there are way too many lines changes if I save spaces as tab for indentation using editor/IDE to insert spaces when I press the TAB key. That is why both spaces and tab were using for the patch. I think it is also important to keep history as we introduce insert spaces as tab for indentation going forward. We could have a project that only changes tab to spaces.
          Hide
          Kristian Waagan added a comment -

          Just in case it wasn't a deliberate choice, the commit added both spaces and tabs for indentation.
          I still don't think the community has agreed formally on this issue, but personally I'm always using spaces for indentation (I configured my editor/IDE to insert spaces when I press the TAB key on my keyboard).

          Show
          Kristian Waagan added a comment - Just in case it wasn't a deliberate choice, the commit added both spaces and tabs for indentation. I still don't think the community has agreed formally on this issue, but personally I'm always using spaces for indentation (I configured my editor/IDE to insert spaces when I press the TAB key on my keyboard).
          Lily Wei made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Lily Wei added a comment -

          Change BaseDataFileFactory.boot() hard-code English message, message order and
          taknt of boot message with DERBY-4716-8.diff subversion 967304

          Show
          Lily Wei added a comment - Change BaseDataFileFactory.boot() hard-code English message, message order and taknt of boot message with DERBY-4716 -8.diff subversion 967304
          Hide
          Knut Anders Hatlen added a comment -

          The latest patch looks fine to me. It looks like you'll get your commit privileges very soon now, so I'll leave this one for you to commit yourself.

          Show
          Knut Anders Hatlen added a comment - The latest patch looks fine to me. It looks like you'll get your commit privileges very soon now, so I'll leave this one for you to commit yourself.
          Hide
          Lily Wei added a comment -

          Thanks Kim for looking into this issue. The documentation issue is like DERBY-4601. Once we have a JIRA for that issue, we shall include all the information on there.

          DERBY-4715-8.diff pass Suites.all and derbyall tests. It is ready to commit. Thanks!!!

          Show
          Lily Wei added a comment - Thanks Kim for looking into this issue. The documentation issue is like DERBY-4601 . Once we have a JIRA for that issue, we shall include all the information on there. DERBY-4715 -8.diff pass Suites.all and derbyall tests. It is ready to commit. Thanks!!!
          Hide
          Kim Haase added a comment -

          It seems likely that the fix to this issue, like the one for DERBY-4601, will affect the messages shown in the Getting Started guide. Perhaps the documentation issue for that issue can reflect these changes too.

          Show
          Kim Haase added a comment - It seems likely that the fix to this issue, like the one for DERBY-4601 , will affect the messages shown in the Getting Started guide. Perhaps the documentation issue for that issue can reflect these changes too.
          Lily Wei made changes -
          Attachment DERBY-4715-8.diff [ 12450085 ]
          Hide
          Lily Wei added a comment -

          Thanks, Knut.
          I attach patch DERBY-4715-8.diff with the same changes as last one and take out the space for "Boot...."
          This is what it looks like now. Please review it:
          ----------------------------------------------------------------
          2010-07-21 18:43:02.968 GMT:
          Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 alp
          stance a816c00e-0129-f650-e20c-000000c1b4f8
          on database directory C:\derby\trunk\testtmp\test with class loader sun.misc.Laun
          r@11b86e7
          Loaded from file:/C:/derby/trunk/jars/sane/derby.jar
          java.vendor=Sun Microsystems Inc.
          java.runtime.version=1.6.0_20-b02
          Database Class Loader started - derby.database.classpath=''

          2010-07-21 18:43:06.484 GMT:
          Shutting down instance a816c00e-0129-f650-e20c-000000c1b4f8 with class loader sun.
          lassLoader@11b86e7
          ----------------------------------------------------------------
          The tests are still running.

          Show
          Lily Wei added a comment - Thanks, Knut. I attach patch DERBY-4715 -8.diff with the same changes as last one and take out the space for "Boot...." This is what it looks like now. Please review it: ---------------------------------------------------------------- 2010-07-21 18:43:02.968 GMT: Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 alp stance a816c00e-0129-f650-e20c-000000c1b4f8 on database directory C:\derby\trunk\testtmp\test with class loader sun.misc.Laun r@11b86e7 Loaded from file:/C:/derby/trunk/jars/sane/derby.jar java.vendor=Sun Microsystems Inc. java.runtime.version=1.6.0_20-b02 Database Class Loader started - derby.database.classpath='' 2010-07-21 18:43:06.484 GMT: Shutting down instance a816c00e-0129-f650-e20c-000000c1b4f8 with class loader sun. lassLoader@11b86e7 ---------------------------------------------------------------- The tests are still running.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Lily. I think the latest changes look good. I agree that removing the initial space in that message sounds like a good idea.

          Show
          Knut Anders Hatlen added a comment - Thanks Lily. I think the latest changes look good. I agree that removing the initial space in that message sounds like a good idea.
          Lily Wei made changes -
          Attachment DERBY-4715-7.diff [ 12450062 ]
          Hide
          Lily Wei added a comment -

          Thank Knut for reviewing the patch.

          "with class loader" comes straight after "on database directory (...)" now.
          And, logMsg(jvmVersion) is after printed the boot message

          I did not take out the space for "Booting ..." message. Should I go ahead and take it out?

          I am running tests now.

          This is the message on my screen, please review it:
          ----------------------------------------------------------------
          2010-07-21 15:45:10.935 GMT:
          Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 al
          pha - (965674M): instance a816c00e-0129-f5ae-0a4d-000000c1b4f8
          on database directory C:\derby\trunk\testtmp\test2 with class loader sun.misc.Lau
          ncher$AppClassLoader@11b86e7
          Loaded from file:/C:/derby/trunk/jars/sane/derby.jar
          java.vendor=Sun Microsystems Inc.
          java.runtime.version=1.6.0_20-b02
          Database Class Loader started - derby.database.classpath=''

          2010-07-21 15:45:14.145 GMT:
          Shutting down instance a816c00e-0129-f5ae-0a4d-000000c1b4f8 with class loader sun.
          misc.Launcher$AppClassLoader@11b86e7
          ----------------------------------------------------------------

          Show
          Lily Wei added a comment - Thank Knut for reviewing the patch. "with class loader" comes straight after "on database directory (...)" now. And, logMsg(jvmVersion) is after printed the boot message I did not take out the space for "Booting ..." message. Should I go ahead and take it out? I am running tests now. This is the message on my screen, please review it: ---------------------------------------------------------------- 2010-07-21 15:45:10.935 GMT: Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 al pha - (965674M): instance a816c00e-0129-f5ae-0a4d-000000c1b4f8 on database directory C:\derby\trunk\testtmp\test2 with class loader sun.misc.Lau ncher$AppClassLoader@11b86e7 Loaded from file:/C:/derby/trunk/jars/sane/derby.jar java.vendor=Sun Microsystems Inc. java.runtime.version=1.6.0_20-b02 Database Class Loader started - derby.database.classpath='' 2010-07-21 15:45:14.145 GMT: Shutting down instance a816c00e-0129-f5ae-0a4d-000000c1b4f8 with class loader sun. misc.Launcher$AppClassLoader@11b86e7 ----------------------------------------------------------------
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Lily.

          After the last patch, the message looks like this:

          Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 alpha - (1): instance a816c00e-0129-f436-4fd9-00000348e710
          on database directory /tmp/db
          Loaded from file:/code/derby/trunk0/classes/
          java.vendor=Sun Microsystems Inc.
          java.runtime.version=1.6.0_21-b06 with class loader sun.misc.Launcher$AppClassLoader@17182c1

          I think it would be better if the "with class loader" part came straight after "on database directory (...)" and not on the java.runtime.version line.

          Would it make sense to add an extra parameter for jvmVersion? It feels a bit strange to have jvmVersion as a part of the JarClassPath parameter. Or perhaps it's cleaner to move jvmVersion out of that message altogether, and instead add a call to "logMsg(jvmVersion);" after having printed the boot message?

          Show
          Knut Anders Hatlen added a comment - Thanks Lily. After the last patch, the message looks like this: Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 alpha - (1): instance a816c00e-0129-f436-4fd9-00000348e710 on database directory /tmp/db Loaded from file:/code/derby/trunk0/classes/ java.vendor=Sun Microsystems Inc. java.runtime.version=1.6.0_21-b06 with class loader sun.misc.Launcher$AppClassLoader@17182c1 I think it would be better if the "with class loader" part came straight after "on database directory (...)" and not on the java.runtime.version line. Would it make sense to add an extra parameter for jvmVersion? It feels a bit strange to have jvmVersion as a part of the JarClassPath parameter. Or perhaps it's cleaner to move jvmVersion out of that message altogether, and instead add a call to "logMsg(jvmVersion);" after having printed the boot message?
          Lily Wei made changes -
          Attachment DERBY-4715-6.diff [ 12449931 ]
          Hide
          Lily Wei added a comment -

          Oops! Thank you for point this out.
          I change the order in BaseDataFileFactory.java to reflect the new messages.xml which has text 'Loaded from " And, I add the method in MessageService to accept one more Object (String)
          This is not for commit yet. I am still running tests.

          Show
          Lily Wei added a comment - Oops! Thank you for point this out. I change the order in BaseDataFileFactory.java to reflect the new messages.xml which has text 'Loaded from " And, I add the method in MessageService to accept one more Object (String) This is not for commit yet. I am still running tests.
          Hide
          Knut Anders Hatlen added a comment -

          Sorry, I missed one issue when I looked through the patch:

          In BaseDataFileFactory.java, there's this diff:

          MessageService.getTextMessage(MessageId.STORE_BOOT_MSG,
          jbmsVersion,
          identifier,

          • dataDirectory + " " + readOnlyMsg,
            // cast to Object so we get object hash code
          • (Object) this.getClass().getClassLoader()
            + (Object) this.getClass().getClassLoader(),
            + dataDirectory + " " + readOnlyMsg
            + +"\nLoaded from " + jarCPath + "\n" +jvmVersion
            ));

          Since we hard-code English text as an argument to STORE_BOOT_MSG, only parts of the message will be translated in non-English locales. We should instead add extra arguments to the STORE_BOOT_MSG message and make the hard-coded English strings part of the message text in messages.xml.

          Also, the patch changed the order of the arguments in that call, so now the message looks like this:

          Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 alpha - (1): instance a816c00e-0129-ef61-4e78-00000348df10
          on database directory sun.misc.Launcher$AppClassLoader@13f5d07 with class loader /tmp/testdb

          Note that it says that "sun.misc.Launcher$AppClassLoader@13f5d07" is a database directory and "/tmp/testdb" is a class loader, but it should have been the other way around.

          Show
          Knut Anders Hatlen added a comment - Sorry, I missed one issue when I looked through the patch: In BaseDataFileFactory.java, there's this diff: MessageService.getTextMessage(MessageId.STORE_BOOT_MSG, jbmsVersion, identifier, dataDirectory + " " + readOnlyMsg, // cast to Object so we get object hash code (Object) this.getClass().getClassLoader() + (Object) this.getClass().getClassLoader(), + dataDirectory + " " + readOnlyMsg + +"\nLoaded from " + jarCPath + "\n" +jvmVersion )); Since we hard-code English text as an argument to STORE_BOOT_MSG, only parts of the message will be translated in non-English locales. We should instead add extra arguments to the STORE_BOOT_MSG message and make the hard-coded English strings part of the message text in messages.xml. Also, the patch changed the order of the arguments in that call, so now the message looks like this: Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 alpha - (1): instance a816c00e-0129-ef61-4e78-00000348df10 on database directory sun.misc.Launcher$AppClassLoader@13f5d07 with class loader /tmp/testdb Note that it says that "sun.misc.Launcher$AppClassLoader@13f5d07" is a database directory and "/tmp/testdb" is a class loader, but it should have been the other way around.
          Hide
          Kathey Marsden added a comment -

          Thanks Lily, I will commit first thing Monday if your account does not come through by then.

          Show
          Kathey Marsden added a comment - Thanks Lily, I will commit first thing Monday if your account does not come through by then.
          Hide
          Lily Wei added a comment -

          Thank you, Knut. Yeah! Suites.all and derbyall seem to run fine except:
          1) ttestSetPortPriority(org.apache.derbyTesting.functionTests.tests.derbynet.Serve
          rPropertiesTest)junit.framework.AssertionFailedError: Port 1537 exceeeds expected
          maximum. You may need to update TestConfiguration.MAX_PORTS_USED and the Wiki page
          at http://wiki.apache.org/db-derby/DerbyJUnitTesting if test runs now require mor
          e available ports
          at org.apache.derbyTesting.junit.TestConfiguration.getNextAvailablePort(Te
          stConfiguration.java:1413)
          at org.apache.derbyTesting.functionTests.tests.derbynet.ServerPropertiesTe
          st.ttestSetPortPriority(ServerPropertiesTest.java:445)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.ja
          va:48)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccesso
          rImpl.java:37)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:10
          9)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)

          This failure is a known issue by the group.

          DERBY-4715-5.diff is ready to submit.

          Show
          Lily Wei added a comment - Thank you, Knut. Yeah! Suites.all and derbyall seem to run fine except: 1) ttestSetPortPriority(org.apache.derbyTesting.functionTests.tests.derbynet.Serve rPropertiesTest)junit.framework.AssertionFailedError: Port 1537 exceeeds expected maximum. You may need to update TestConfiguration.MAX_PORTS_USED and the Wiki page at http://wiki.apache.org/db-derby/DerbyJUnitTesting if test runs now require mor e available ports at org.apache.derbyTesting.junit.TestConfiguration.getNextAvailablePort(Te stConfiguration.java:1413) at org.apache.derbyTesting.functionTests.tests.derbynet.ServerPropertiesTe st.ttestSetPortPriority(ServerPropertiesTest.java:445) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.ja va:48) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccesso rImpl.java:37) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:10 9) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) This failure is a known issue by the group. DERBY-4715 -5.diff is ready to submit.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Lily. The latest patch looks fine to me.

          Show
          Knut Anders Hatlen added a comment - Thanks, Lily. The latest patch looks fine to me.
          Lily Wei made changes -
          Attachment DERBY-4715-5.diff [ 12449365 ]
          Hide
          Lily Wei added a comment -

          Thanks Knut for reviewing the patch. I make the following changes in DERBY-4715-5.diff

          • The javadoc for jarClassPath() is a little unclear. I don't understand what's meant by "the path of string" or by "the string of jar file".
            javadoc is changed. I am using similar comment from Main.java. Hope it is clearer now.
          • The javadoc for buildJvmVersion() should make it clear that it returns values of system properties that identify the JVM.
            Add this to the javadoc on buildJvmVersion()
          • buildJvmVersion() has a @param tag, but the method has no parameters.
            It is not there any more.
          • Now that we add a newline character between each property, there's no need to append " " (one space) to each property value.
            Great catch. Change to not append " ".
          • If jarClassPath() had taken a Class argument instead of a String argument, and we called it like "jarCPath = jarClassPath(getClass());", there would be no need to call Class.forName(), and we could remove the catch block for ClassNotFoundException.
            After using getClass() to get the class instance for the BaseDataFileFactory class which is also in derby.jar, no need to call Class.forName() and catch block for ClassNotFoundException. Please review the change.

          I am running tests now.

          Show
          Lily Wei added a comment - Thanks Knut for reviewing the patch. I make the following changes in DERBY-4715 -5.diff The javadoc for jarClassPath() is a little unclear. I don't understand what's meant by "the path of string" or by "the string of jar file". javadoc is changed. I am using similar comment from Main.java. Hope it is clearer now. The javadoc for buildJvmVersion() should make it clear that it returns values of system properties that identify the JVM. Add this to the javadoc on buildJvmVersion() buildJvmVersion() has a @param tag, but the method has no parameters. It is not there any more. Now that we add a newline character between each property, there's no need to append " " (one space) to each property value. Great catch. Change to not append " ". If jarClassPath() had taken a Class argument instead of a String argument, and we called it like "jarCPath = jarClassPath(getClass());", there would be no need to call Class.forName(), and we could remove the catch block for ClassNotFoundException. After using getClass() to get the class instance for the BaseDataFileFactory class which is also in derby.jar, no need to call Class.forName() and catch block for ClassNotFoundException. Please review the change. I am running tests now.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for making these improvements, Lily. A couple more comments to the latest patch:

          • The javadoc for jarClassPath() is a little unclear. I don't understand what's meant by "the path of string" or by "the string of jar file".
          • The javadoc for buildJvmVersion() should make it clear that it returns values of system properties that identify the JVM.
          • buildJvmVersion() has a @param tag, but the method has no parameters.
          • Now that we add a newline character between each property, there's no need to append " " (one space) to each property value.
          • If jarClassPath() had taken a Class argument instead of a String argument, and we called it like "jarCPath = jarClassPath(getClass());", there would be no need to call Class.forName(), and we could remove the catch block for ClassNotFoundException.
          Show
          Knut Anders Hatlen added a comment - Thanks for making these improvements, Lily. A couple more comments to the latest patch: The javadoc for jarClassPath() is a little unclear. I don't understand what's meant by "the path of string" or by "the string of jar file". The javadoc for buildJvmVersion() should make it clear that it returns values of system properties that identify the JVM. buildJvmVersion() has a @param tag, but the method has no parameters. Now that we add a newline character between each property, there's no need to append " " (one space) to each property value. If jarClassPath() had taken a Class argument instead of a String argument, and we called it like "jarCPath = jarClassPath(getClass());", there would be no need to call Class.forName(), and we could remove the catch block for ClassNotFoundException.
          Lily Wei made changes -
          Attachment DERBY-4715-4.diff [ 12449286 ]
          Hide
          Lily Wei added a comment -

          Thanks Knut, Kathey and Kristian for all the commets.
          I made the following changes and attach DERBY-4715-4.diff patch for review.

          Comments:

          • The methods jarClassPath() and buildJvmVersion() have different indentation than the other methods.
            Changes according to the current file indentation. Right now, the method starts at space 5 while the context starts at space 9.
          • If formatURL() gets an IOException, it returns the string "IOException". Perhaps it should also return the exception's message text?
            It will be return message text for IOException now.
          • I don't follow all the logic in formatURL(). Could you add some comments that explain the various transformations? In my environment, just printing what URL.toString() returns gives clear enough information:

          file:/code/derby/trunk0/classes/ or
          file:/code/derby/trunk0/jars/sane/derby.jar
          Great point. I always believe coding less is better. Use URL.toString() in the code.

          • For readability, I think it would be better to separate the properties with a newline character instead of a space.
            The property is separated by a newline instead of a space.
          • The comments in the policy files say "Add for DERBY-4715". I think it would be better if they said why the permission was needed since we include them as templates in the distribution and refer to them from the documentation[1], so they are expected to be read by users. Perhaps something like "getProtectionDomain is an optional permission needed for printing classpath information to derby.log."
            All the comment relate to java.lang.RuntimePermission is using this comment.
          • In server.policy and template.policy, the new permission is added in the section that is labeled "These permissions are needed for everyday, embedded Derby usage." My understanding of the patch is that the new permission is optional, and embedded Derby will work fine without it, it just won't print the classpath to derby.log. If it's so, I'd suggest moving it to a less prominent section of the policy files.
            Put the java.lang.RuntimePermission "getProtectionDomain" and the comment in its own section
          • Similarly, in AssertFailureTest.policy, the permission is added under the label "These are the ones that matter", but AssertFailureTest runs fine without it.
            Make similar change as above and place in less prominent section.

          Regarding the public methods in utility classes, there are two new methods - jarClassPatha and buildJvmVersion after removing formatUrl() in BaseDataFileFactory.java. They are not so generic to be used or provide a lot of value in the utility classes.

          I am running tests against this patch now.

          Show
          Lily Wei added a comment - Thanks Knut, Kathey and Kristian for all the commets. I made the following changes and attach DERBY-4715 -4.diff patch for review. Comments: The methods jarClassPath() and buildJvmVersion() have different indentation than the other methods. Changes according to the current file indentation. Right now, the method starts at space 5 while the context starts at space 9. If formatURL() gets an IOException, it returns the string "IOException". Perhaps it should also return the exception's message text? It will be return message text for IOException now. I don't follow all the logic in formatURL(). Could you add some comments that explain the various transformations? In my environment, just printing what URL.toString() returns gives clear enough information: file:/code/derby/trunk0/classes/ or file:/code/derby/trunk0/jars/sane/derby.jar Great point. I always believe coding less is better. Use URL.toString() in the code. For readability, I think it would be better to separate the properties with a newline character instead of a space. The property is separated by a newline instead of a space. The comments in the policy files say "Add for DERBY-4715 ". I think it would be better if they said why the permission was needed since we include them as templates in the distribution and refer to them from the documentation [1] , so they are expected to be read by users. Perhaps something like "getProtectionDomain is an optional permission needed for printing classpath information to derby.log." All the comment relate to java.lang.RuntimePermission is using this comment. In server.policy and template.policy, the new permission is added in the section that is labeled "These permissions are needed for everyday, embedded Derby usage." My understanding of the patch is that the new permission is optional, and embedded Derby will work fine without it, it just won't print the classpath to derby.log. If it's so, I'd suggest moving it to a less prominent section of the policy files. Put the java.lang.RuntimePermission "getProtectionDomain" and the comment in its own section Similarly, in AssertFailureTest.policy, the permission is added under the label "These are the ones that matter", but AssertFailureTest runs fine without it. Make similar change as above and place in less prominent section. Regarding the public methods in utility classes, there are two new methods - jarClassPatha and buildJvmVersion after removing formatUrl() in BaseDataFileFactory.java. They are not so generic to be used or provide a lot of value in the utility classes. I am running tests against this patch now.
          Hide
          Kristian Waagan added a comment -

          Regarding the public methods in utility classes, I think it is fine to create (and use them) if they add value, but the utility method should not include the call to doPrivileged. Instead, the calling code (which is hopefully not easily accessible and controllable for external users) should call the utility method inside a doPrivileged-block.
          The above approach isn't as compact, but Kathey is right that adding public (static) utility methods with doPrivileged-blocks is dangerous.

          Show
          Kristian Waagan added a comment - Regarding the public methods in utility classes, I think it is fine to create (and use them) if they add value, but the utility method should not include the call to doPrivileged. Instead, the calling code (which is hopefully not easily accessible and controllable for external users) should call the utility method inside a doPrivileged-block. The above approach isn't as compact, but Kathey is right that adding public (static) utility methods with doPrivileged-blocks is dangerous.
          Hide
          Kathey Marsden added a comment -

          I have not looked at the updated patch yet, but I know that sometimes it is tempting to put methods requiring permissions in public methods of utility classes, but it is generally not a good idea to do so as it allows external users to access those permissions granted to Derby.

          Show
          Kathey Marsden added a comment - I have not looked at the updated patch yet, but I know that sometimes it is tempting to put methods requiring permissions in public methods of utility classes, but it is generally not a good idea to do so as it allows external users to access those permissions granted to Derby.
          Hide
          Knut Anders Hatlen added a comment -

          A couple comments in addition to what Bryan said:

          • The methods jarClassPath() and buildJvmVersion() have different indentation than the other methods.
          • If formatURL() gets an IOException, it returns the string "IOException". Perhaps it should also return the exception's message text?
          • I don't follow all the logic in formatURL(). Could you add some comments that explain the various transformations? In my environment, just printing what URL.toString() returns gives clear enough information:

          file:/code/derby/trunk0/classes/ or
          file:/code/derby/trunk0/jars/sane/derby.jar

          • For readability, I think it would be better to separate the properties with a newline character instead of a space.
          • The comments in the policy files say "Add for DERBY-4715". I think it would be better if they said why the permission was needed since we include them as templates in the distribution and refer to them from the documentation[1], so they are expected to be read by users. Perhaps something like "getProtectionDomain is an optional permission needed for printing classpath information to derby.log."
          • In server.policy and template.policy, the new permission is added in the section that is labeled "These permissions are needed for everyday, embedded Derby usage." My understanding of the patch is that the new permission is optional, and embedded Derby will work fine without it, it just won't print the classpath to derby.log. If it's so, I'd suggest moving it to a less prominent section of the policy files.
          • Similarly, in AssertFailureTest.policy, the permission is added under the label "These are the ones that matter", but AssertFailureTest runs fine without it.

          [1] http://db.apache.org/derby/docs/dev/adminguide/tadminnetservcustom.html

          Show
          Knut Anders Hatlen added a comment - A couple comments in addition to what Bryan said: The methods jarClassPath() and buildJvmVersion() have different indentation than the other methods. If formatURL() gets an IOException, it returns the string "IOException". Perhaps it should also return the exception's message text? I don't follow all the logic in formatURL(). Could you add some comments that explain the various transformations? In my environment, just printing what URL.toString() returns gives clear enough information: file:/code/derby/trunk0/classes/ or file:/code/derby/trunk0/jars/sane/derby.jar For readability, I think it would be better to separate the properties with a newline character instead of a space. The comments in the policy files say "Add for DERBY-4715 ". I think it would be better if they said why the permission was needed since we include them as templates in the distribution and refer to them from the documentation [1] , so they are expected to be read by users. Perhaps something like "getProtectionDomain is an optional permission needed for printing classpath information to derby.log." In server.policy and template.policy, the new permission is added in the section that is labeled "These permissions are needed for everyday, embedded Derby usage." My understanding of the patch is that the new permission is optional, and embedded Derby will work fine without it, it just won't print the classpath to derby.log. If it's so, I'd suggest moving it to a less prominent section of the policy files. Similarly, in AssertFailureTest.policy, the permission is added under the label "These are the ones that matter", but AssertFailureTest runs fine without it. [1] http://db.apache.org/derby/docs/dev/adminguide/tadminnetservcustom.html
          Hide
          Bryan Pendleton added a comment -

          I think the patch looks good; the format of the message is OK by me.

          The new utility methods in BaseDataFileFactory seem very useful,
          but it doesn't seem quite right to place them in BaseDataFileFactory.
          It seems like maybe they should go into a package like
          org.apache.derby.iapi.util. Perhaps we need a UriUtil class there?

          Show
          Bryan Pendleton added a comment - I think the patch looks good; the format of the message is OK by me. The new utility methods in BaseDataFileFactory seem very useful, but it doesn't seem quite right to place them in BaseDataFileFactory. It seems like maybe they should go into a package like org.apache.derby.iapi.util. Perhaps we need a UriUtil class there?
          Hide
          Lily Wei added a comment -

          Thanks Bryan for catching that. Do you see any potential issue with the patch?

          Show
          Lily Wei added a comment - Thanks Bryan for catching that. Do you see any potential issue with the patch?
          Bryan Pendleton made changes -
          Summary Wrie jvm information and path of derby.jar to derby.log Write jvm information and path of derby.jar to derby.log
          Hide
          Lily Wei added a comment -

          derbyall suites run clean too. Granting permissions to Derby need some write up for runtime permission on getProtectionDomain and maybe for PropertyPermission on java.runtime.version and java.fullversion. I will issue another JIRA to address the documentation issue relate to this JIRA.

          Show
          Lily Wei added a comment - derbyall suites run clean too. Granting permissions to Derby need some write up for runtime permission on getProtectionDomain and maybe for PropertyPermission on java.runtime.version and java.fullversion. I will issue another JIRA to address the documentation issue relate to this JIRA.
          Lily Wei made changes -
          Attachment DERBY-4715-3.diff [ 12449145 ]
          Hide
          Lily Wei added a comment -

          Thanks to Kathey for point out BaseDataFileFactory.buildJvmVersion should use AccessController.doPrivileged to handle derby under a security manager.
          I check the derby.log to make sure boot and communicate that there is not sufficient permission to report the information for derby.jar and java version.

          Suites.all test suite passed. I am running derbyall now.

          Show
          Lily Wei added a comment - Thanks to Kathey for point out BaseDataFileFactory.buildJvmVersion should use AccessController.doPrivileged to handle derby under a security manager. I check the derby.log to make sure boot and communicate that there is not sufficient permission to report the information for derby.jar and java version. Suites.all test suite passed. I am running derbyall now.
          Hide
          Lily Wei added a comment -

          derbyall test passed with the patch too.

          Show
          Lily Wei added a comment - derbyall test passed with the patch too.
          Lily Wei made changes -
          Attachment DERBY-4715-2.diff [ 12448858 ]
          Hide
          Lily Wei added a comment -

          Thanks to Kathey. DERBY-4715-2.diff patch will boot and communicate there is not sufficient permission to derby.log for derby.jar. If there is no permission for print out java.runtime.version or java.fullversion, there will only print java vendor information to derby.log

          Suites.all test is passed. It is running derbyall test suite. Please review the patch

          Show
          Lily Wei added a comment - Thanks to Kathey. DERBY-4715 -2.diff patch will boot and communicate there is not sufficient permission to derby.log for derby.jar. If there is no permission for print out java.runtime.version or java.fullversion, there will only print java vendor information to derby.log Suites.all test is passed. It is running derbyall test suite. Please review the patch
          Hide
          Kathey Marsden added a comment -

          Hi Lily,

          Could you make sure something reasonable happens if there is no permission in the policy file for reading the system properties, getting the source location etc. We should still boot and communicate that there is not sufficient permission to report the information but not in an alarming way.

          Show
          Kathey Marsden added a comment - Hi Lily, Could you make sure something reasonable happens if there is no permission in the policy file for reading the system properties, getting the source location etc. We should still boot and communicate that there is not sufficient permission to report the information but not in an alarming way.
          Hide
          Lily Wei added a comment -

          The tests passed on derbyall and Suites.allpackages

          Show
          Lily Wei added a comment - The tests passed on derbyall and Suites.allpackages
          Lily Wei made changes -
          Issue & fix info [Newcomer] [Newcomer, Patch Available]
          Lily Wei made changes -
          Attachment DERBY-4715-1.diff [ 12448539 ]
          Attachment derby.log [ 12448540 ]
          Hide
          Lily Wei added a comment -

          Write java vendor, java runtime version and java fullversion, the path of derby.jar to derby.log. Please review regarding:
          1. Does the output looks okay?
          2. Whether this will create too long of output for derby.log in general.

          Here is a sample of how derby.log looks now:
          2010-07-01 21:38:01.173 GMT:
          Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 al
          pha - (959745M): instance a816c00e-0129-8ff1-e2bd-000000132a78
          on database directory sun.misc.Launcher$AppClassLoader@69ba69ba with class loader
          C:\derby\trunk\testtmp\test
          Loaded from C:\derby\trunk\jars\sane\derby.jar
          java.vendor=IBM Corporation java.runtime.version=pwi32dev-20090707 (SR10 ) J2RE 1.
          5.0 IBM J9 2.3 Windows Vista x86-32 j9vmwi3223-20090707 (JIT enabled)
          J9VM - 20090706_38445_lHdSMr
          JIT - 20090623_1334_r8
          GC - 200906_09java.fullversion=J2RE 1.5.0 IBM J9 2.3 Windows Vista x86-32 j9vmwi
          3223-20090707 (JIT enabled)
          J9VM - 20090706_38445_lHdSMr
          JIT - 20090623_1334_r8
          GC - 200906_09
          ^M
          Database Class Loader started - derby.database.classpath=''^M

          Show
          Lily Wei added a comment - Write java vendor, java runtime version and java fullversion, the path of derby.jar to derby.log. Please review regarding: 1. Does the output looks okay? 2. Whether this will create too long of output for derby.log in general. Here is a sample of how derby.log looks now: 2010-07-01 21:38:01.173 GMT: Booting Derby version The Apache Software Foundation - Apache Derby - 10.7.0.0 al pha - (959745M): instance a816c00e-0129-8ff1-e2bd-000000132a78 on database directory sun.misc.Launcher$AppClassLoader@69ba69ba with class loader C:\derby\trunk\testtmp\test Loaded from C:\derby\trunk\jars\sane\derby.jar java.vendor=IBM Corporation java.runtime.version=pwi32dev-20090707 (SR10 ) J2RE 1. 5.0 IBM J9 2.3 Windows Vista x86-32 j9vmwi3223-20090707 (JIT enabled) J9VM - 20090706_38445_lHdSMr JIT - 20090623_1334_r8 GC - 200906_09java.fullversion=J2RE 1.5.0 IBM J9 2.3 Windows Vista x86-32 j9vmwi 3223-20090707 (JIT enabled) J9VM - 20090706_38445_lHdSMr JIT - 20090623_1334_r8 GC - 200906_09 ^M Database Class Loader started - derby.database.classpath=''^M
          Lily Wei made changes -
          Assignee Lily Wei [ lilywei ]
          Lily Wei made changes -
          Field Original Value New Value
          Link This issue is part of DERBY-1272 [ DERBY-1272 ]
          Lily Wei created issue -

            People

            • Assignee:
              Lily Wei
              Reporter:
              Lily Wei
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development