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.pa...@oracle.com>; Andrew John Hughes > <gnu.and...@redhat.com>; core-libs-...@openjdk.java.net; i18n- > d...@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.and...@redhat.com> > >> Sent: Saturday, July 6, 2019 9:53 PM > >> To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs- > >> d...@openjdk.java.net; i18n-dev@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 > >>
