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
