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

[Roger Riggs]

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