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

naoto.sato at oracle.com naoto.sato at oracle.com
Thu Jan 16 21:08:53 UTC 2020 

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?

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

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

More information about the i18n-dev mailing list