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
