Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-17 Thread Roger Riggs
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

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-16 Thread naoto . sato
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

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-16 Thread Roger Riggs
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. 73: include the failed exit

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-15 Thread naoto . sato
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

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-15 Thread Roger Riggs
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

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-15 Thread naoto . sato
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

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-15 Thread Roger Riggs
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) HijrahChronology.java:

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-14 Thread Joe Wang
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)

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-14 Thread naoto . sato
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

Re: [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-14 Thread Joe Wang
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

[15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

2020-01-14 Thread naoto . sato
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