Hi Naoto,
Looks good. Thanks for the updates.
Roger
On 1/16/20 4:08 PM, naoto.s...@oracle.com wrote:
Hi Roger,
Thanks. My comments are embedded below.
On 1/16/20 12:06 PM, Roger Riggs wrote:
Hi Naoto,
A couple of comments in the tests.
HijrahConfigTest:
72: Since onExit() starts a task in some executor and in some context,
its not clear that an exception thrown in that task will be
reported.
Use the normal p.waitFor() and check the exit code.
Hmm, I was using the example in onExit() method, which reads:
---
API Note:
Using onExit is an alternative to waitFor that enables both additional
concurrency and convenient access to the result of the Process.
---
So "alternative" is not "interchangeable" in this context?
onExit is useful to process the exit of a Process asynchronously.
73: include the failed exit value in the exception; It may have some
use in debugging.
Anyway, I changed the above code to p.waitFor() for this reason.
HijrahConfigCheck:
41: The data structure is a set; no entry can appear twice; there's
no point to the text
or the value of count should be included in the exception
At first I thought the same, but realized there could be a error case
where "calendar type" in returned objects are the same, but objects
themselves are different (thus the Set could accommodate both), so the
test.
I suppose that is up to the Chronology .equals method of each
Chronology, I'd expect equals
to be checking all the fields of a Chronology.
44: typo: Instatiation
48: if they are different, print the two chronologies
hijrah-config-Hijrah-test_islamic-test.properties:
I would include a comment to say the data does not represent an
actual calendar.
Corrected.
Roger
p.s.
!! tests.Helper is a very uninformative class name (but that's not
your doing).
"Helper" is not helping in that regard :-)
Updated webrev: http://cr.openjdk.java.net/~naoto/8187987/webrev.03/
Naoto
On 1/15/20 7:49 PM, naoto.s...@oracle.com wrote:
Updated:
https://cr.openjdk.java.net/~naoto/8187987/webrev.02/
The change includes the new naming convention, reduction of
properties files reading to once, and utilization of logging.
Naoto
On 1/15/20 12:37 PM, Roger Riggs wrote:
Hi,
On 1/15/20 3:06 PM, naoto.s...@oracle.com wrote:
Hi Roger,
Thank you for the review. Please find my comments below:
On 1/15/20 10:30 AM, Roger Riggs wrote:
Hi Naoto,
Is it correct to say that there is no impact on startup until
there is an explicit reference to HijrahChronology?
It would seem that the registering HijrahChronology would trigger
all the work and that happens when Chronology is initialized.
(see below)
What I meant in the reply to Joe's email was that the data
validity check done in loadCalendarData(), e.g., year value check,
etc. which are not done at class init. But you are correct that
the properties files are read twice (below). I thought it was fine
as this is not a common case (not happened before, to be precise).
Until it get used, there will still be a probe of the filesystem to
see if a config directory/file exists
that would impact every startup. I don't see a way to mitigate that
without making the config more complex.
HijrahChronology.java:
291-295: Since registerCustomChrono is the only place where
CONF_PATH is used,
do all the work, including the property lookup in that method.
836: If other chronologies are built-in this code will need to
be changed.
Can it do the getResourceAsStream first in all cases and fall
back to /conf/chronology?
Yes, it would have to be changed if we support built-in type other
than umalqura. But I would think other code would have to anyways.
I think we can take advantage of the fact that umalqura is the
only built-in, and others come from /conf/chronology so that extra
fallback is avoided.
Possibly, but could/would be more localized and consist of only the
registerChronology call.
855: Since all the loading is triggered from a static
initializer, is there really any point in throwing an exception.
More useful would be a logging message (assuming logging is
allowed early in startup when Chronology is initialized)
Good point. I will replace UncheckedIOException with logging.
1054: readConfigProperties: The case for the other
HijrahChronologies delays loading the data until it is needed.
This is doing the work to read the file and create the properties
instance but then discards it to be read a second time later.
Perhaps we need to specify that the config file name includes
both the id and type so it can be registered without reading the
file.
Should the file name like "Hijrah-config-<id>_<type>.properties"
work?
yes, its worth checking the valid characters in id and type.
Thanks, Roger
BTW, the test drive can be implemented using testng, only the test
itself is easier as main().
Naoto
Regards, Roger
On 1/14/20 9:37 PM, Joe Wang wrote:
On 1/14/20 6:04 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Thank you for the review. Please find my comments below:
On 1/14/20 3:35 PM, Joe Wang wrote:
Hi Naoto,
Since it's dealing with non-standard properties files, is
there a need to verify the files? The constructor
(HijrahChronology) does check whether the id or type is empty.
If there is no existing process to validate, it's probably not
worth it to spend time as it's rare and it's config time.
IIUC, the idea is to create the instance at class loading time
(thus the faster the better) and cache it, then check the
validity later at actual method invocation (cf.
checkCalendarInit() method).
Make sense.
The test summary states "Test image creation", it may be
better to say sth. like verifies custom configuration?
Good catch! I was simply copying some portion from other test
case. Corrected:
https://cr.openjdk.java.net/~naoto/8187987/webrev.01/
Looks good to me.
Best regards,
Joe
Naoto
Best,
Joe
On 1/14/20 8:35 AM, naoto.s...@oracle.com wrote:
Hi,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8187987
The proposed CSR and changeset are located at:
CSR: https://bugs.openjdk.java.net/browse/JDK-8236810
Webrev: https://cr.openjdk.java.net/~naoto/8187987/webrev.00/
The spec of java.time.chrono.HijrahChronology suggests that
it could load custom variants of Hijirah calendar type using
properties files. However it does not work as designed. This
change intends to make it work.
Naoto
