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

[naoto.sato at oracle.com]

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