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

Tue Jul 9 13:58:58 UTC 2019

+1

On 7/9/19 9:07 AM, naoto.sato at oracle.com wrote:
> Looks good, Ramanand.
>
> Naoto
>
> On 7/9/19 5:09 AM, Ramanand Patil wrote:
>> Hi Naoto,
>> Thank you for the review. Revised webrev: 
>> http://cr.openjdk.java.net/~rpatil/8224560/webrev.02/
>>
>>
>> I plan to push the changes tomorrow, if there are no further comments.
>>
>>
>> Regards,
>> Ramanand.
>>
>>> -----Original Message-----
>>> From: Naoto Sato
>>> Sent: Monday, July 8, 2019 11:19 PM
>>> To: Ramanand Patil <ramanand.patil at oracle.com>; Andrew John Hughes
>>> <gnu.andrew at redhat.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
>>>
>>> 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/uti
>>>>> l/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/sha
>>>> re/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/s
>>>>>> ha
>>>>>> 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
>>>>>