<i18n dev> RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13

[naoto.sato at oracle.com]

Hi Ramanand,

As to the change in ZoneInfoFile.java, I would put that special handling 
of Gaza/Hebron in line 577, as well as merging the comment in 575,576, 
so that it won't duplicate the code.

Otherwise looks good.

Naoto

On 7/8/19 3:35 AM, Ramanand Patil wrote:
> Hi Andrew,
> Thank you for your review.
> Updated webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.01/
> 
> Regards,
> Ramanand.
> 
>> -----Original Message-----
>> From: Andrew John Hughes <gnu.andrew at redhat.com>
>> Sent: Saturday, July 6, 2019 9:53 PM
>> To: Ramanand Patil <ramanand.patil at oracle.com>; core-libs-
>> dev at openjdk.java.net; i18n-dev at openjdk.java.net
>> Subject: Re: <i18n dev> RFR: 8224560: (tz) Upgrade time-zone data to
>> tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-
>> 13
>>
>>
>>
>> On 05/07/2019 15:16, Ramanand Patil wrote:
>>> Hi all,
>>> Please review the patch for tzdata2019a integration into jdk project.
>>> Webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.00/
>>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8224560
>>> https://bugs.openjdk.java.net/browse/JDK-8225580
>>>
>>> Summary:
>>> - The fix contains cumulative tzdata changes from tzdata2018i and
>> tzdata2019a, as tzdata2018i was not integrated into jdk/jdk earlier.
>>> - In JDK-13/14, multiple failures were seen during integration of tzdata2018i
>> (JDK-8225580), those are fixed now. Many thanks to Naoto for providing a fix
>> for this in CLDRConverter.java.
>>
>> I would guess this is due to the CLDR update (JDK-8221432: Upgrade CLDR to
>> Version 35.1) in OpenJDK 13, making TimeZone.getAvailableIDs
>> inappropriate in some way?
>>
>> Fix looks good. One minor change:
>>
>> +            AVAILABLE_TZIDS = new
>> + HashSet(ZoneId.getAvailableZoneIds());
>>
>> is missing the <String> or <>:
>>
>> +            AVAILABLE_TZIDS = new
>> + HashSet<>(ZoneId.getAvailableZoneIds());
>>
>> Will this fix also resolve JDK-8225580? If so, it's probably worth mentioning
>> both bug IDs in the commit.
> Yes, this fix will also resolve JDK-8225580, hence included in the subject line.
> And thank you, I will add both bug IDs in the commit message.
>>
>>> - There are 2 type of test failures in TestZoneInfo310.java file, which are
>> solved in this patch by providing workarounds, But a permanent fix needs to
>> be added in future for the same. Below are the 2 bugs created to track the
>> development on it:
>>> 	1. https://bugs.openjdk.java.net/browse/JDK-8223388:
>> TestZoneInfo310.java fails post tzdata2018i integration:
>>> This failure is seen for the TimeZones which are having zone rules defined
>> till year 2037 or beyond. For now, the failing zones are skipped.
>>> The supporting test class ZoneInfo.java has maxYear defined
>> http://hg.openjdk.java.net/jdk/jdk/file/d01b345865d7/test/jdk/sun/util/cale
>> ndar/zi/Zoneinfo.java#l40, changing this max value greater than the zone
>> rule's last year also fixes the issue, but further investigation is needed on why
>> this boundary condition is affecting the test behavior.
>>
>> I wonder if 2037 is in someway related to the rollover of 32-bit time values?
> I think, not directly related but how the test and JDK handles these values.
> In JDK, the transitions beyond 2037 are delegated to SimpleTimeZone, and I think the test somehow miscalculates it.
> http://hg.openjdk.java.net/jdk/jdk/file/5919b273def6/src/java.base/share/classes/sun/util/calendar/ZoneInfo.java#l48
> I think, I should re-visit and see if these test are really needed now. As per the long standing bug JDK-8166983 suggestion was to remove the whole tests from test/sun/util/calendar/zi
>>
>>> 	2. JDK-8227293: https://bugs.openjdk.java.net/browse/JDK-8227293
>> TestZoneInfo310.java fails post tzdata2019a integration for Palestine zone
>> rules:
>>> There are many hacks and assumptions in the class
>>> sun.util.calendar.ZoneInfoFile.java. This issue looks because of the
>>> code starting from here:
>>> http://hg.openjdk.java.net/jdk/jdk/file/963924f1c891/src/java.base/sha
>>> re/classes/sun/util/calendar/ZoneInfoFile.java#l552
>>> There is an assumption where the transition date is >=24,(line 577 and 599)
>> it assumes it is the "last" rule, but it is not last rule in case of Asia/Gaza and
>> Asia/Hebron zones.
>>> For now, I have fixed these 2 problematic timezones by overwriting the
>>> assumption made on line 577, where date of month dom = startRule.dom; I
>> think, overriding of the second jdk hack on line 599 is not required as the
>> "dom" is calculated from the last rule there. Keeping this bug open as we
>> need to find a generic solution for this issue, without hard-coding the values
>> and adding specific time zone names in exclusion as seen in many places in
>> this class file.
>>>
>>> - The patch has passed all the related testing including JCK tests.
>>>
>>>
>>> Regards,
>>> Ramanand.
>>>
>>>
>>>
>>>
>>>
>>
>> Looks good to me, with the above minor adjustment.
>>
>> Thanks,
>> --
>> Andrew :)
>>
>> Senior Free Java Software Engineer
>> Red Hat, Inc. (http://www.redhat.com)
>>
>> PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint
>> = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
>> https://keybase.io/gnu_andrew
>>