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

Roger Riggs Roger.Riggs at oracle.com
Fri Jan 17 21:07:37 UTC 2020 

Hi Naoto,

Looks good.  Thanks for the updates.

Roger


On 1/16/20 4:08 PM, naoto.sato at 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 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?
onExit is useful to process the exit of a Process asynchronously.
>
>>
>> 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.
I suppose that is up to the Chronology .equals method of each 
Chronology, I'd expect equals
to be checking all the fields of a Chronology.
>
>>
>> 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