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