[
https://issues.apache.org/jira/browse/DERBY-4715?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lily Wei updated DERBY-4715:
----------------------------
Attachment: DERBY-4715-4.diff
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.
> Write jvm information and path of derby.jar to derby.log
> --------------------------------------------------------
>
> Key: DERBY-4715
> URL: https://issues.apache.org/jira/browse/DERBY-4715
> Project: Derby
> Issue Type: Bug
> Components: Miscellaneous
> Affects Versions: 10.6.1.0
> Reporter: Lily Wei
> Assignee: Lily Wei
> Priority: Minor
> Fix For: 10.7.0.0
>
> Attachments: DERBY-4715-1.diff, DERBY-4715-2.diff, DERBY-4715-3.diff,
> DERBY-4715-4.diff, derby.log
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
