Thanks Serguei!
Can I get one more review please?
thanks,
Chris
On 4/7/20 10:54 PM, [email protected] wrote:
Chris,
Refactored version looks nice.
I was thinking to suggest the same in previous review but decided
there is not a big profit for small blocks of code.
But it is obvious now that it is clearly better. :)
Thanks,
Serguei
On 4/7/20 22:15, Chris Plummer wrote:
Hello,
Please review the following:
https://bugs.openjdk.java.net/browse/JDK-8242162
http://cr.openjdk.java.net/~cjplummer/8242162/webrev.00
http://cr.openjdk.java.net/~cjplummer/8242162/webrev.01
Note there are two webrevs. You can skip the first if you want. It's
the initial straight forward implementation. After that I did
refactoring, so it might be easier if you looked at webrev.00 first
without the refactoring, but I will be pushing webrev.01.
[Serguei, regarding the refactoring you suggested, I didn't find it
was worth doing for the execution of the commands since there are 4
of them, and they all have subtle differences that makes it hard to
refactor in a productive way. I did refactor comparing the lists and
counting the lists.]
Regarding the change in LingeredAppSysProps.java, I ran into an issue
when I added the clhsdb test. I was suddenly seeing an extra
property. It was user.timezone. I did some research and found that
user.timezone is kind of special. If not set on the command line it
will not be created until someone calls TimeZone.getDefault(). What’s
interesting is that when using the Attach API to get the sysprops
(using jinfo or jcmd), the first time you do this you don’t get back
user.timezone, but repeat the command and you do. Somehow the first
time the command is executed it triggers the initialization of
user.timezone, but only after the sysprops result was generated. So
in the test LingeredAppSysProps.main() was printing the properties
without use.name, and so was "jhsdb jinfo" and "jinfo". But the last
"jinfo" command triggered the initialization of user.timezone, so it
turned up in the subsequent run of "clhsdb sysprops", and thus the
count in that list was one higher than the rest. By forcing the
initialization of user.timezone at the start of
LingeredAppSysProps.main(), now user.timezone shows up in all 4 lists.
thanks,
Chris
