<i18n dev> [15] RFR: 8187987: Add a mechanism to configure custom variants in HijrahChronology

[naoto.sato at oracle.com]

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).

> 
> 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.

> 
> 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?

Naoto

> 
> Regards, Roger
> 
> 
> On 1/14/20 9:37 PM, Joe Wang wrote:
>>
>>
>> On 1/14/20 6:04 PM, naoto.sato at 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.sato at 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
>>>>
>>
>