RE: RFR: 8231098: (tz) Upgrade time-zone data to tzdata2019c
Hi Naoto and Martin, Thank you very much for your reviews. Hi Martin, Naoto is correct, the jdk build will fail for the update releases when using vanguard format, since the fix from JDK-8212970 is not yet backported. Since, jdk14 has this fix, my current patch uses vanguard format tzdata. I am planning to backport JDK-8212970 along with the tzdata2019c integration into the update releases, if there are no major issues. I have already seen the 11u backport request from Christoph for-8212970. Regards, Ramanand. > -Original Message- > From: Naoto Sato > Sent: Tuesday, September 17, 2019 11:25 PM > To: Martin Buchholz > Cc: Ramanand Patil ; core-libs-dev d...@openjdk.java.net>; i18n-dev > Subject: Re: RFR: 8231098: (tz) Upgrade time-zone data to > tzdata2019c > > On 9/17/19 10:14 AM, Martin Buchholz wrote: > > > > > > On Tue, Sep 17, 2019 at 9:45 AM > <mailto:naoto.s...@oracle.com>> wrote: > > > > +1 > > > > On 9/17/19 8:29 AM, Martin Buchholz wrote: > > > Looks good to me. > > > At Google we also integrated tzdata2019c, and it was uneventful > > (good!). > > > But we're still using rearguard format. > > > The vanguard/rearguard distinction is a source of errors, so it > > should be > > > made clear what format is being used to import the files. > > > If you have a script to update the jdk sources, perhaps it should be > > > checked in to openjdk? > > > If these files are in vanguard format, is there a trap for those > > of us > > > doing backports to jdks that only support rearguard? > > > > Can't think of any off the top of my head, Martin. The build tool > > > > > > Which build tool? TzdbZoneRulesCompiler? > > Yes. > > > > > internally converts vanguard data into rearguard compatible, by > > adjusting the standard offsets, and build into tzdb.dat (which should > > even be compatible to prior JDK runtimes). > > > > > > So ... a change like this one is created by copying selected vanguard > > format files from the tzdata source distribution into openjdk, and > > then TzdbZoneRulesCompiler converts to rearguard format internally? > > At Google, we chose to run tzdata's own tool to convert to rearguard > > format and then check in those data files into the jdk source tree. > > The tool will not convert the file format to rearguard. Read the vanguard > format file, then converts the data into tzdb.dat that can be recognized by > prior runtimes. > > > > > I worry that when other engineers backport to older releases, if only > > the data files are copied, then conversion to rearguard format will > > not happen, with bad results. > > If an engineer only backports the vanguard files into older releases (before > the vanguard support), I don't think the JDK build will succeed. > Build will fail on creating tzdb.dat file. > > Naoto
RFR: 8231098: (tz) Upgrade time-zone data to tzdata2019c
Hi all, Please review the patch for tzdata2019c integration into jdk project(jdk-14). Webrev: http://cr.openjdk.java.net/~rpatil/8231098/14/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8231098 The patch has passed all the related testing including JCK. Regards, Ramanand.
[13u]: RFR: 8228469: (tz) Upgrade time-zone data to tzdata2019b
Hi all, Please review the patch for jdk13u backport of tzdata2019b integration into jdk: Webrev: http://cr.openjdk.java.net/~rpatil/8228469/13u/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8228469 The patch is not clean because: It uses "rearguard" format tzdata from IANA (instead of vanguard format), as the enhancement bug[1] is not fixed in jdk13u. The patch has passed all the related testing including JCK. Note: As per the mail from tzdata maintainers[2] there is a possibility that Brazil might not abolish DST. [1] https://bugs.openjdk.java.net/browse/JDK-8212970 [2] https://mm.icann.org/pipermail/tz/2019-July/028344.html Regards, Ramanand.
RE: RFR: 8228469: (tz) Upgrade time-zone data to tzdata2019b
Thank you Martin and Naoto for your reviews. Hi Martin, Currently, we are evaluating the approach for update releases. But in my opinion, it is better to migrate to vanguard format. Regards, Ramanand. From: Martin Buchholz Sent: Tuesday, August 6, 2019 1:57 AM To: Ramanand Patil Cc: core-libs-dev ; i18n-dev Subject: Re: RFR: 8228469: (tz) Upgrade time-zone data to tzdata2019b Thanks for the update and redundancy removal. Looks good to me. What is the recommendation for older releases? Migrate to vanguard format by backporting recent changes or stay on rearguard forever? On Mon, Aug 5, 2019 at 1:28 AM Ramanand Patil <mailto:ramanand.pa...@oracle.com> wrote: Hi all, Please review the patch for tzdata2019b integration into jdk project(jdk-14). Webrev: http://cr.openjdk.java.net/~rpatil/8228469/14/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8228469 Summary: - This patch uses "vanguard" format tzdata from IANA (instead of rearguard format), as the enhancement bug[1] is fixed now. Many thanks to Naoto for this. - As per the latest changes to Palestine zone rules, the hard-coded checks for "Asia/Gaza" and "Asia/Hebron" are no more needed in ZoneInfoFile.java. Because of those checks test/jdk/sun/util/calendar/zi/TestZoneInfo310.java was failing as mentioned in the bug comment[3]. - test/jdk/java/util/TimeZone/TimeZoneTest.java is updated to set ZoneDescriptor.daylight value to "false" for BET zone id. - The patch has passed all the related testing including JCK. Notes: - As per the mail from tzdata maintainers[2] there is a possibility that Brazil might not abolish DST. - Since the enhancement bug[1] is not fixed in update releases, jdk13u will still use the rearguard format tzdata. A separate review request will be sent for the same. [1] https://bugs.openjdk.java.net/browse/JDK-8212970 [2] https://mm.icann.org/pipermail/tz/2019-July/028344.html [3] https://bugs.openjdk.java.net/browse/JDK-8228469?focusedCommentId=14281498=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14281498 Regards, Ramanand.
RFR: 8228469: (tz) Upgrade time-zone data to tzdata2019b
Hi all, Please review the patch for tzdata2019b integration into jdk project(jdk-14). Webrev: http://cr.openjdk.java.net/~rpatil/8228469/14/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8228469 Summary: - This patch uses "vanguard" format tzdata from IANA (instead of rearguard format), as the enhancement bug[1] is fixed now. Many thanks to Naoto for this. - As per the latest changes to Palestine zone rules, the hard-coded checks for "Asia/Gaza" and "Asia/Hebron" are no more needed in ZoneInfoFile.java. Because of those checks test/jdk/sun/util/calendar/zi/TestZoneInfo310.java was failing as mentioned in the bug comment[3]. - test/jdk/java/util/TimeZone/TimeZoneTest.java is updated to set ZoneDescriptor.daylight value to "false" for BET zone id. - The patch has passed all the related testing including JCK. Notes: - As per the mail from tzdata maintainers[2] there is a possibility that Brazil might not abolish DST. - Since the enhancement bug[1] is not fixed in update releases, jdk13u will still use the rearguard format tzdata. A separate review request will be sent for the same. [1] https://bugs.openjdk.java.net/browse/JDK-8212970 [2] https://mm.icann.org/pipermail/tz/2019-July/028344.html [3] https://bugs.openjdk.java.net/browse/JDK-8228469?focusedCommentId=14281498=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14281498 Regards, Ramanand.
RE: RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13
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 ; Andrew John Hughes > ; core-libs-dev@openjdk.java.net; i18n- > d...@openjdk.java.net > Subject: Re: 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 > >> Sent: Saturday, July 6, 2019 9:53 PM > >> To: Ramanand Patil ; core-libs- > >> d...@openjdk.java.net; i18n-...@openjdk.java.net > >> Subject: Re: 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 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. A
RE: RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13
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 > Sent: Saturday, July 6, 2019 9:53 PM > To: Ramanand Patil ; core-libs- > d...@openjdk.java.net; i18n-...@openjdk.java.net > Subject: Re: 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 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 >
RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13
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. - 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/calendar/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. 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/share/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.
RE: [13] RFR: JDK-8206879: Currency decimal marker incorrect for Peru
Hi Deepak, Minor, but it will be good if you change the test case name to something like TestPeruDecimalFormat.java or TestPeruCurrencyDecimalFormat instead of just using BugID. Regards, Ramanand. -Original Message- From: Naoto Sato Sent: Friday, May 10, 2019 6:12 PM To: Deepak Kejriwal ; i18n-...@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: [13] RFR: JDK-8206879: Currency decimal marker incorrect for Peru Hi Deepak, here are my comments. - FormatData_es_PE.java: Modify the copyright year to 2019. - Changes in "LocaleData" may be placed at the bottom of the file, explicitly indicating it is changed with 8206879. Please follow the similar changes' format. - Bug8206879.java does not have proper copyright header. Naoto On 5/10/19 4:25 AM, Deepak Kejriwal wrote: > Hello, > > > > Please review the fix to the following issue: > > https://bugs.openjdk.java.net/browse/JDK-8206879 > > > > The proposed fix is located at: > > http://cr.openjdk.java.net/~dkejriwal/8206879/webrev.00/ > > > > Summary > > In case of JRE locale provider, for Peru comma (,) is used as decimal marker > which is incorrect. The fix is to correct decimal marker for Peru from comma > (,) to dot (.). > > > > Regard, > > Deepak > > >
RE: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633
Hi Christoph, I have suggested the changes considering the fact that this is not a clean backport. Both the source and test files are manually edited and review is requested for the same. Thank you for reminding about jdk8u-fix-request label, I think Deepak will add it. Regards, Ramanand. > -Original Message- > From: Langer, Christoph > Sent: Monday, March 25, 2019 12:42 PM > To: Deepak Kejriwal > Cc: Ramanand Patil ; Naoto Sato > ; core-libs-dev ; > jdk8u-...@openjdk.java.net > Subject: RE: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633 > > Hi there, > > since this is a downport for jdk/jdk, I think the copyright headers should be > the same as upstream. > > So, for src/share/classes/java/time/format/DateTimeFormatterBuilder.java, > you should take 2018 as copyright year. For the test the headers look correct. > > As for jdk8u push: Will you push it to OpenJDK 8 updates or to Oracle 8 > updates. For the former, you'll have to request downport by setting the > jdk8u-fix-request label in the bugs. > > Best regards > Christoph > > > -Original Message- > > From: jdk8u-dev On Behalf Of > > Ramanand Patil > > Sent: Montag, 25. März 2019 07:38 > > To: Deepak Kejriwal ; Naoto Sato > > ; core-libs-dev > > ; jdk8u-...@openjdk.java.net > > Subject: RE: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633 > > > > Hi Deepak, > > > > In particular, the test TestDateTimeFormatterBuilderWithLocale.java > > should have only one copyright year i.e. 2019, since this is a new > > file in jdk8u-dev repos. Also I think, you can omit the second > > copyright info(from line no. 24) for the same reason. > > > > > > Note: I am not a reviewer for JDK 8 Updates Project. > > > > Regards, > > Ramanand. > > > > > -Original Message- > > > From: Deepak Kejriwal > > > Sent: Monday, March 25, 2019 10:11 AM > > > To: Naoto Sato ; core-libs-dev > > d...@openjdk.java.net>; jdk8u-...@openjdk.java.net > > > Subject: RE: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633 > > > > > > Hi Naoto, > > > > > > Thanks for review. I will update the copyright information and push > > > the changes. > > > > > > Regards, > > > Deepak > > > > > > > > > -Original Message- > > > From: Naoto Sato > > > Sent: Friday, March 22, 2019 10:59 PM > > > To: Deepak Kejriwal ; core-libs-dev > > > ; jdk8u-...@openjdk.java.net > > > Subject: Re: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633 > > > > > > Hi Deepak, > > > > > > Please modify the copyright year accordingly. Otherwise it looks > > > good to > > me. > > > > > > Naoto > > > > > > On 3/22/19 8:51 AM, Deepak Kejriwal wrote: > > > > Hi All, > > > > > > > > > > > > > > > > Please review the back port of fix for JDK-8042131 and JDK-8210633 > > > > to 8u- > > > dev:- > > > > > > > > > > > > > > > > JBS report: https://bugs.openjdk.java.net/browse/JDK-8042131 > > > > > > > >https://bugs.openjdk.java.net/browse/JDK-8210633 > > > > > > > > > > > > > > > > Webrev: > > http://cr.openjdk.java.net/~rpatil/8042131_8210633/webrev.00/ > > > > > > > > > > > > > > > > Master bug change set: > > > http://hg.openjdk.java.net/jdk/jdk/rev/f2d94a0619a2 > > > > > > > > http://hg.openjdk.java.net/jdk/jdk/rev/a0426bc28519 > > > > > > > > Summary: > > > > The backport of fix for both bugs JDK-8042131 (from 11u) and JDK- > > 8210633 > > > (from 12u) are not clean backport. Changes for file > > > "DateTimeFormatterBuilder.java" are manually merged. Since, test > > > file "TestDateTimeFormatterBuilderWithLocale.java" is new in 8u > > > release only test cases modified as part for JDK-8042131 and JDK-8210633 > are added. > > > > > > > > All tests are run against the changes and found to be passing. > > > > > > > > Regards, > > > > > > > > Deepak > > > > > > > > > > > >
RE: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633
Hi Deepak, In particular, the test TestDateTimeFormatterBuilderWithLocale.java should have only one copyright year i.e. 2019, since this is a new file in jdk8u-dev repos. Also I think, you can omit the second copyright info(from line no. 24) for the same reason. Note: I am not a reviewer for JDK 8 Updates Project. Regards, Ramanand. > -Original Message- > From: Deepak Kejriwal > Sent: Monday, March 25, 2019 10:11 AM > To: Naoto Sato ; core-libs-dev d...@openjdk.java.net>; jdk8u-...@openjdk.java.net > Subject: RE: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633 > > Hi Naoto, > > Thanks for review. I will update the copyright information and push the > changes. > > Regards, > Deepak > > > -Original Message- > From: Naoto Sato > Sent: Friday, March 22, 2019 10:59 PM > To: Deepak Kejriwal ; core-libs-dev libs-...@openjdk.java.net>; jdk8u-...@openjdk.java.net > Subject: Re: RFR: JDK8U Backport of JDK-8042131 and JDK-8210633 > > Hi Deepak, > > Please modify the copyright year accordingly. Otherwise it looks good to me. > > Naoto > > On 3/22/19 8:51 AM, Deepak Kejriwal wrote: > > Hi All, > > > > > > > > Please review the back port of fix for JDK-8042131 and JDK-8210633 to 8u- > dev:- > > > > > > > > JBS report: https://bugs.openjdk.java.net/browse/JDK-8042131 > > > >https://bugs.openjdk.java.net/browse/JDK-8210633 > > > > > > > > Webrev: http://cr.openjdk.java.net/~rpatil/8042131_8210633/webrev.00/ > > > > > > > > Master bug change set: > http://hg.openjdk.java.net/jdk/jdk/rev/f2d94a0619a2 > > > > http://hg.openjdk.java.net/jdk/jdk/rev/a0426bc28519 > > > > Summary: > > The backport of fix for both bugs JDK-8042131 (from 11u) and JDK-8210633 > (from 12u) are not clean backport. Changes for file > "DateTimeFormatterBuilder.java" are manually merged. Since, test file > "TestDateTimeFormatterBuilderWithLocale.java" is new in 8u release only > test cases modified as part for JDK-8042131 and JDK-8210633 are added. > > > > All tests are run against the changes and found to be passing. > > > > Regards, > > > > Deepak > > > > > >
RFR: 8213085: (tz) Upgrade time-zone data to tzdata2018g
Hi all, Please review the latest TZDATA integration (tzdata2018g) into JDK12. Bug: https://bugs.openjdk.java.net/browse/JDK-8213085 Webrev: http://cr.openjdk.java.net/~rpatil/8213085/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RE: RFR: 8213016: (tz) Upgrade time-zone data to tzdata2018f
Thank you Martin and Naoto for your reviews. Unfortunately, I have to discard this review and start a new review for tzdata2018g, since 2018g is already released now. Regards, Ramanand. > -Original Message- > From: Naoto Sato > Sent: Saturday, October 27, 2018 1:41 AM > To: Martin Buchholz ; Ramanand Patil > > Cc: core-libs-dev ; i18n-dev d...@openjdk.java.net> > Subject: Re: RFR: 8213016: (tz) Upgrade time-zone data to > tzdata2018f > > +1 > > Naoto > > On 10/26/18 12:38 PM, Martin Buchholz wrote: > > Looks good to me. > > > > On Fri, Oct 26, 2018 at 11:30 AM, Ramanand Patil > > > > wrote: > > > >> Hi all, > >> Please review the latest TZDATA integration (tzdata2018f) into JDK12. > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8213016 > >> Webrev: http://cr.openjdk.java.net/~rpatil/8213016/webrev.00/ > >> > >> All the TimeZone related tests are passed after integration. > >> > >> Note: > >> The used tzdata files are from the rearguard version of the > >> tzdata2018f release with the patch applied given by the IANA > maintainers[1]. > >> This is done to avoid the value "25:00" from the Japanese ZoneRule. > >> But, as proposed by Stephen[2], a fix from JDK side is also required > >> which will be tracked via a new bug [3]. > >> > >> [1] https://mm.icann.org/pipermail/tz/2018-October/027032.html > >> [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018- > >> October/056167.html > >> [3] https://bugs.openjdk.java.net/browse/JDK-8212970 > >> > >> > >> Regards, > >> Ramanand. > >>
RFR: 8213016: (tz) Upgrade time-zone data to tzdata2018f
Hi all, Please review the latest TZDATA integration (tzdata2018f) into JDK12. Bug: https://bugs.openjdk.java.net/browse/JDK-8213016 Webrev: http://cr.openjdk.java.net/~rpatil/8213016/webrev.00/ All the TimeZone related tests are passed after integration. Note: The used tzdata files are from the rearguard version of the tzdata2018f release with the patch applied given by the IANA maintainers[1]. This is done to avoid the value "25:00" from the Japanese ZoneRule. But, as proposed by Stephen[2], a fix from JDK side is also required which will be tracked via a new bug [3]. [1] https://mm.icann.org/pipermail/tz/2018-October/027032.html [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056167.html [3] https://bugs.openjdk.java.net/browse/JDK-8212970 Regards, Ramanand.
RE: Time-zone database issues
Hi Naoto, Thank you for filing the bug. As far as tzdata2018f release is concerned I am going ahead with the integration, with the help of the patch provided here- https://mm.icann.org/pipermail/tz/2018-October/027032.html Which avoids 25:00 in rearguard format. [ https://bugs.openjdk.java.net/browse/JDK-8213016 ] Regards, Ramanand. > -Original Message- > From: Naoto Sato > Sent: Thursday, October 25, 2018 8:48 PM > To: Stephen Colebourne ; core-libs-dev d...@openjdk.java.net> > Subject: Re: Time-zone database issues > > Thanks, Stephen. > > I filed an issue for your suggestion: > > https://bugs.openjdk.java.net/browse/JDK-8212970 > > I will need to look into the issue, but so far as I understand, will it be > fine to > modify the offending transition date to the next day for 2018f's immediate > issue? > > Naoto > > On 10/22/18 3:25 PM, Stephen Colebourne wrote: > > The IANA time-zone database [1] provides details of how time-zones > > change over time. The latest release - 2018f - cannot be processed > > successfully by the current JDK parser [2]. A workaround exists > > however unlike previous cases of tzdb incompatibility, this time it > > makes sense for the JDK parser and API to be changed. > > > > Problem > > --- > > The JDK parser and API make the assumption that a time-zone change can > > occur at any LocalTime or midnight-end-of-day, ie. from 00:00 to > > 24:00. Unfortunately, the tzdb source files allow (and now include) > > rules outside those valid values, in this case a value of 25:00. > > Specifically, the rule that causes problems says that clocks change at > > 25:00 on the first Saturday on or after the 8th of September. > > > > In the current problematic case, the rule can be rewritten to say that > > clocks change at 01:00 on the first Sunday on or after the 9th of > > September. However, there are cases where it is difficult to > > impossible to rewrite the rule (such as 25:00 on the last Saturday in > > a month, difficult because it goes into the next month). > > > > Proposed solution > > > > > > Fixing the parser to handle values like 25:00 would be relatively > > easy. However, these rules are also exposed in the public API of > > java.time.zone.ZoneOffsetTransitionRule [3]. Currently this class has > > methods `getLocalTime()` and `isMidnightEndOfDay()`. These would need > > to be deprecated and replaced by a new method `getLocalTimeDuration()` > > (or some other name) that returns an instance of `Duration`. > > > > A user of ThreeTen-Backport [4] has provided a branch to do this, so I > > know the change to be possible. However, since I have looked at the > > code I cannot implement the change in OpenJDK (compromised IP). It > > needs a cleanroom implementation by someone else. > > > > Is there agreement on the need for change? Is anyone (Oracle or > > otherwise) willing to volunteer do the work? > > > > thanks > > Stephen > > > > > > [1] https://www.iana.org/time-zones > > [2] https://bugs.openjdk.java.net/browse/JDK-8212684 > > [3] > > https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time > > /zone/ZoneOffsetTransitionRule.html > > [4] https://www.threeten.org/threetenbp/ > >
RFR[jdk8u-dev]: 8134124: sun/security/tools/jarsigner/warnings.sh fails when using Hindi locale
Hi all, Please review this trivial test fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8134124 Webrev: http://cr.openjdk.java.net/~rpatil/8134124/webrev.00/ The test is made locale independent now. Regards, Ramanand.
RFR: 8203233: (tz) Upgrade time-zone data to tzdata2018e
Hi all, Please review the latest TZDATA integration (tzdata2018e) to JDK. Bug: https://bugs.openjdk.java.net/browse/JDK-8203233 Webrev: http://cr.openjdk.java.net/~rpatil/8203233/webrev.00/ All the TimeZone related tests are passed after integration. Note: Using the rearguard version of the tzdata, to avoid the negative save value problem. https://mm.icann.org/pipermail/tz/2018-February/026203.html Regards, Ramanand.
RFR: 8200359: (tz) Upgrade time-zone data to tzdata2018d
Hi all, Please review the latest TZDATA integration (tzdata2018d) into JDK11. Bug: https://bugs.openjdk.java.net/browse/JDK-8200359 Webrev: http://cr.openjdk.java.net/~rpatil/8200359/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR[JDK10]: 8195837: (tz) Support tzdata2018c
Hi all, Please review the latest TZDATA integration (tzdata2018c) into JDK10. Bug: https://bugs.openjdk.java.net/browse/JDK-8195837 Webrev: http://cr.openjdk.java.net/~rpatil/8195837/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RE: RFR: JDK8u Backport of 8153955: increase java.util.logging.FileHandler MAX_LOCKS limit
Thank you Daniel and Sean for your reviews. Regards, Ramanand. > -Original Message- > From: Daniel Fuchs > Sent: Wednesday, December 20, 2017 2:43 PM > To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs-dev d...@openjdk.java.net> > Subject: Re: RFR: JDK8u Backport of 8153955: increase > java.util.logging.FileHandler MAX_LOCKS limit > > Hi Ramanand, > > On 18/12/2017 11:41, Ramanand Patil wrote: > > Hi all, > > Please review the fix for JDK8u Backport of > https://bugs.openjdk.java.net/browse/JDK-8153955 > > Backport Bug: https://bugs.openjdk.java.net/browse/JDK-8161266 > > Webrev: http://cr.openjdk.java.net/~rpatil/8161266/webrev.00/ > > Looks good to me as well. > > best regards, > > -- daniel > > > > > Summary(also added to backport bug description): > > The fix from JDK9 cannot be backported as is into the jdk8u and earlier > update releases, since it contains JDK API spec changes. > > In JDK9 the fix is added by altering the FileHandler spec, which introduces > > a > new configurable property "java.util.logging.FileHandler.maxLocks" to > java.util.logging.FileHandler, defined in .../conf/logging.properties. > > The solution proposed for update releases is: > > To introduce an internal JDK implementation specific property- > "jdk.internal.FileHandlerLogging.maxLocks" which will be used to > control/override FileHandler's MAX_LOCKS value. The default value of the > maxLocks (100) will be retained if this new System property is not set. The > new property is read only once during FileHandler class initialization. > > > > > > Regards, > > Ramanand. > > >
RFR: JDK8u Backport of 8153955: increase java.util.logging.FileHandler MAX_LOCKS limit
Hi all, Please review the fix for JDK8u Backport of https://bugs.openjdk.java.net/browse/JDK-8153955 Backport Bug: https://bugs.openjdk.java.net/browse/JDK-8161266 Webrev: http://cr.openjdk.java.net/~rpatil/8161266/webrev.00/ Summary(also added to backport bug description): The fix from JDK9 cannot be backported as is into the jdk8u and earlier update releases, since it contains JDK API spec changes. In JDK9 the fix is added by altering the FileHandler spec, which introduces a new configurable property "java.util.logging.FileHandler.maxLocks" to java.util.logging.FileHandler, defined in .../conf/logging.properties. The solution proposed for update releases is: To introduce an internal JDK implementation specific property- "jdk.internal.FileHandlerLogging.maxLocks" which will be used to control/override FileHandler's MAX_LOCKS value. The default value of the maxLocks (100) will be retained if this new System property is not set. The new property is read only once during FileHandler class initialization. Regards, Ramanand.
RE: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c
Thank you Martin for your review. For the java file updates, currently I don’t use any script but just simple “find and replace” whenever possible and rest is manual. I run all the TZ related tests locally on Ubuntu and use JPRT for all other platforms. Regards, Ramanand. From: Martin Buchholz [mailto:marti...@google.com] Sent: Wednesday, November 08, 2017 12:18 AM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: Naoto Sato <naoto.s...@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; i18n-...@openjdk.java.net Subject: Re: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c Looks good to me too. I've always wondered about how the corresponding java source files are maintained. Is it all manual or do y'all have some magic script to help keep IANA data and java data aligned? Do we need more testing that mistakes don't creep in? On Tue, Nov 7, 2017 at 3:41 AM, Ramanand Patil mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com> wrote: Hi Naoto, Thank you for pointing that. I have modified both the affected files(ZoneName.java from src and test). Here is the updated Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.01/ Regards, Ramanand. > -Original Message- > From: Naoto Sato > Sent: Tuesday, November 07, 2017 5:18 AM > To: Ramanand Patil "mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com>; core-libs-dev > HYPERLINK "mailto:d...@openjdk.java.net"d...@openjdk.java.net>; HYPERLINK > "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net > Subject: Re: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c > and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c > > Hi Ramanand, > > In java/time/format/ZoneName.java, should the zid for Africa_Central map > to "Africa/Maputo"? > > Naoto > > On 11/6/17 6:06 AM, Ramanand Patil wrote: > > Hi all, > > Please review the latest TZDATA integration (tzdata2017c) to JDK10. > > Bugs: > > https://bugs.openjdk.java.net/browse/JDK-8190258 > > https://bugs.openjdk.java.net/browse/JDK-8190259 > > > > Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/ > > > > All the TimeZone related tests are passed after integration. > > > > Regards, > > Ramanand. > >
RE: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c
Hi Naoto, Thank you for pointing that. I have modified both the affected files(ZoneName.java from src and test). Here is the updated Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.01/ Regards, Ramanand. > -Original Message- > From: Naoto Sato > Sent: Tuesday, November 07, 2017 5:18 AM > To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs-dev d...@openjdk.java.net>; i18n-...@openjdk.java.net > Subject: Re: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c > and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c > > Hi Ramanand, > > In java/time/format/ZoneName.java, should the zid for Africa_Central map > to "Africa/Maputo"? > > Naoto > > On 11/6/17 6:06 AM, Ramanand Patil wrote: > > Hi all, > > Please review the latest TZDATA integration (tzdata2017c) to JDK10. > > Bugs: > > https://bugs.openjdk.java.net/browse/JDK-8190258 > > https://bugs.openjdk.java.net/browse/JDK-8190259 > > > > Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/ > > > > All the TimeZone related tests are passed after integration. > > > > Regards, > > Ramanand. > >
JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c
Hi all, Please review the latest TZDATA integration (tzdata2017c) to JDK10. Bugs: https://bugs.openjdk.java.net/browse/JDK-8190258 https://bugs.openjdk.java.net/browse/JDK-8190259 Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RE: tzdata2017c is out
Hi Martin,
Thank you very much for the heads-up. I have filed a bug for the same:
https://bugs.openjdk.java.net/browse/JDK-8190259
Thank you for the patch as well. This will be integrated into JDK along with
the tzdata2017c.
Regards,
Ramanand.
From: Martin Buchholz [mailto:marti...@google.com]
Sent: Tuesday, October 24, 2017 2:05 AM
To: i18n-...@openjdk.java.net; core-libs-dev <core-libs-dev@openjdk.java.net>;
Ramanand Patil <ramanand.pa...@oracle.com>; Stephen Colebourne
<scolebou...@joda.org>
Subject: tzdata2017c is out
tzdata2017c came out today, and the elves at Google are working hard to
incorporate changes in hours or days instead of weeks or quarters. One thing
we noticed is that one of the java.time tck tests was broken by tzdata rewrite
of history. Here's the fix we're applying (s/1879/1892/g):
@@ -941,21 +941,21 @@
}
public void test_Apia_jumpForwardOverInternationalDateLine_P12_to_M12() {
- // transition occurred at 1879-07-04T00:00+12:33:04
+ // transition occurred at 1892-07-04T00:00+12:33:04
ZoneRules test = pacificApia();
- Instant instantBefore = LocalDate.of(1879, 7,
2).atStartOfDay(ZoneOffset.UTC).toInstant();
+ Instant instantBefore = LocalDate.of(1892, 7,
2).atStartOfDay(ZoneOffset.UTC).toInstant();
ZoneOffsetTransition trans = test.nextTransition(instantBefore);
- assertEquals(trans.getDateTimeBefore(), LocalDateTime.of(1879, 7, 5,
0, 0));
- assertEquals(trans.getDateTimeAfter(), LocalDateTime.of(1879, 7, 4, 0,
0));
+ assertEquals(trans.getDateTimeBefore(), LocalDateTime.of(1892, 7, 5,
0, 0));
+ assertEquals(trans.getDateTimeAfter(), LocalDateTime.of(1892, 7, 4, 0,
0));
assertEquals(trans.isGap(), false);
assertEquals(trans.isOverlap(), true);
assertEquals(trans.isValidOffset(ZoneOffset.ofHoursMinutesSeconds(+12,
33, 4)), true);
assertEquals(trans.isValidOffset(ZoneOffset.ofHoursMinutesSeconds(-11,
-26, -56)), true);
assertEquals(trans.getDuration(), Duration.ofHours(-24));
- assertEquals(trans.getInstant(), LocalDateTime.of(1879, 7, 4, 0,
0).toInstant(ZoneOffset.ofHoursMinutesSeconds(-11, -26, -56)));
+ assertEquals(trans.getInstant(), LocalDateTime.of(1892, 7, 4, 0,
0).toInstant(ZoneOffset.ofHoursMinutesSeconds(-11, -26, -56)));
- ZonedDateTime zdt = ZonedDateTime.of(1879, 7, 4, 23, 0, 0, 0,
ZoneId.of("Pacific/Apia"));
- assertEquals(zdt.plusHours(2).toLocalDateTime(),
LocalDateTime.of(1879, 7, 4, 1, 0, 0));
+ ZonedDateTime zdt = ZonedDateTime.of(1892, 7, 4, 23, 0, 0, 0,
ZoneId.of("Pacific/Apia"));
+ assertEquals(zdt.plusHours(2).toLocalDateTime(),
LocalDateTime.of(1892, 7, 4, 1, 0, 0));
}
//-
RFR: JDK8U Backport of 8185346: Relax RMI Registry Serial Filter to allow arrays of any type
Hi All, Please review this webrev for jdk8u backport. Webrev: http://cr.openjdk.java.net/~rpatil/8185346/jdk8u-dev/webrev.00/ Main Bug: https://bugs.openjdk.java.net/browse/JDK-8185346 JDK10 review thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-August/048738.html JDK10 changeset: http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/6256e94781f5 The usage of shared secrets is removed as compared to the original fix since the ObjectInputFilter is accessible in the jdk.internal.misc package. Regards, Ramanand.
RE: RFR: 8177449: (tz) Support tzdata2017b
Thank you Martin for the quick review. Regards, Ramanand. From: Martin Buchholz [mailto:marti...@google.com] Sent: Thursday, March 30, 2017 12:05 AM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: i18n-...@openjdk.java.net; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8177449: (tz) Support tzdata2017b Looks good to me! On Wed, Mar 29, 2017 at 11:31 AM, Ramanand Patil mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com> wrote: Hi all, Please review the latest TZDATA integration (tzdata2017b) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8177449 Webrev: http://cr.openjdk.java.net/~rpatil/8177449/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR: 8177449: (tz) Support tzdata2017b
Hi all, Please review the latest TZDATA integration (tzdata2017b) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8177449 Webrev: http://cr.openjdk.java.net/~rpatil/8177449/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR: 8176044: (tz) Support tzdata2017a
Hi all, Please review the latest TZDATA integration (tzdata2017a) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8176044 Webrev: http://cr.openjdk.java.net/~rpatil/8176044/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RE: RFR: 8173943: Change error reporting of LauncherHelper to include actual Error class name
Thank you Alan and Kumar. I have just pushed the fix. Regards, Ramanand. -Original Message- From: Kumar Srinivasan Sent: Monday, February 06, 2017 9:54 PM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8173943: Change error reporting of LauncherHelper to include actual Error class name +1 Kumar On 2/5/2017 11:14 AM, Ramanand Patil wrote: > Hi all, > Please review the following trivial bug fix: > Bug: https://bugs.openjdk.java.net/browse/JDK-8173943 > Webrev: http://cr.openjdk.java.net/~rpatil/8173943/webrev.00/ > > LinkageError message added by - > https://bugs.openjdk.java.net/browse/JDK-8167063 is updated to include the > Error Class name. > > Regards, > Ramanand. >
RE: RFR: 8173943: Change error reporting of LauncherHelper to include actual Error class name
Hi David,
I have updated the bug report with the below sample output error message as
suggested.
Here is the sample error output for comparison(taken from the test case of
JDK-8167063):
1. Before Fixing JDK-8167063:
Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.IllegalAccessError: superclass access
check failed: class pkg.b.Bar (in module mod.b) cannot access class pkg.a.Foo
(in module mod.a) because module mod.a does not export pkg.a to module mod.b
at java.lang.ClassLoader.defineClass1(java.base@9-ea/Native Method)
at java.lang.ClassLoader.defineClass(java.base@9-ea/ClassLoader.java:947)
at java.lang.ClassLoader.defineClass(java.base@9-ea/ClassLoader.java:1024)
at
java.security.SecureClassLoader.defineClass(java.base@9-ea/SecureClassLoader.java:182)
at
jdk.internal.loader.BuiltinClassLoader.defineClass(java.base@9-ea/BuiltinClassLoader.java:512)
at
jdk.internal.loader.BuiltinClassLoader.lambda$findClassInModuleOrNull$2(java.base@9-ea/BuiltinClassLoader.java:449)
at java.security.AccessController.doPrivileged(java.base@9-ea/Native Method)
at
jdk.internal.loader.BuiltinClassLoader.findClassInModuleOrNull(java.base@9-ea/BuiltinClassLoader.java:450)
at
jdk.internal.loader.BuiltinClassLoader.findClass(java.base@9-ea/BuiltinClassLoader.java:354)
at java.lang.ClassLoader.loadLocalClass(java.base@9-ea/ClassLoader.java:536)
at java.lang.Class.forName(java.base@9-ea/Class.java:446)
at
sun.launcher.LauncherHelper.loadModuleMainClass(java.base@9-ea/LauncherHelper.java:544)
at
sun.launcher.LauncherHelper.checkAndLoadMain(java.base@9-ea/LauncherHelper.java:496)
2. After Fixing JDK-8167063:
Error: Unable to load main class pkgB.ClassB from module mod.b
superclass access check failed: class pkgB.ClassB (in module mod.b) cannot
access class pkgA.ClassA (in module mod.a) because module mod.a does not export
pkgA to module mod.b
[Here the complete stacktrace was removed to be consistent with the error
handling mechanism of LauncherHelper, where only the important error message is
printed and the complete stack trace is shown in diagnostic ("-Xdiag") mode.]
3. After fixing the current Bug( JDK-8173943 ):
Error: Unable to load main class pkgB.ClassB from module mod.b
java.lang.IllegalAccessError: superclass access check failed: class pkgB.ClassB
(in module mod.b) cannot access class pkgA.ClassA (in module mod.a) because
module mod.a does not export pkgA to module mod.b
Regards,
Ramanand.
-Original Message-
From: David Holmes
Sent: Monday, February 06, 2017 3:10 AM
To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8173943: Change error reporting of LauncherHelper to include
actual Error class name
Hi Ramanand,
On 6/02/2017 5:14 AM, Ramanand Patil wrote:
> Hi all,
> Please review the following trivial bug fix:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8173943
> Webrev: http://cr.openjdk.java.net/~rpatil/8173943/webrev.00/
>
> LinkageError message added by -
> https://bugs.openjdk.java.net/browse/JDK-8167063 is updated to include the
> Error Class name.
For changes like this it would be useful to show (in the RFR and/or the bug
report) how the error messages appear before and after the change as that is
what we need to verify. And in this case we need to compare against what was
reported prior to JDK-8167063 being fixed.
Thanks,
David
-
> Regards,
> Ramanand.
>
RFR: 8173943: Change error reporting of LauncherHelper to include actual Error class name
Hi all, Please review the following trivial bug fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8173943 Webrev: http://cr.openjdk.java.net/~rpatil/8173943/webrev.00/ LinkageError message added by - https://bugs.openjdk.java.net/browse/JDK-8167063 is updated to include the Error Class name. Regards, Ramanand.
RE: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized
Thank you Kumar and Alan. Just pushed the fix. All the related tests from "core" and "pit" testset are passed. Regards, Ramanand. -Original Message- From: Kumar Srinivasan Sent: Tuesday, January 31, 2017 9:53 PM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: Alan Bateman <alan.bate...@oracle.com>; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized Approved, please make sure you have run all the jprt -testset pit tests. Kumar On 1/30/2017 5:05 AM, Alan Bateman wrote: > On 30/01/2017 12:11, Ramanand Patil wrote: > >> Thank you Alan. >> >> I have eliminated the inner catch block for LinkageError. Here is the >> updated webrev: >> http://cr.openjdk.java.net/~rpatil/8167063/webrev.04 >> >> > I think this looks right now. > > -Alan --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
RE: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized
Thank you Alan. I have eliminated the inner catch block for LinkageError. Here is the updated webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.04 Regards, Ramanand. -Original Message- From: Alan Bateman Sent: Sunday, January 29, 2017 1:06 AM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized On 27/01/2017 12:52, Ramanand Patil wrote: > Thank you Alan and Kumar for your review. > Here is the updated webrev: > http://cr.openjdk.java.net/~rpatil/8167063/webrev.03 > > Changes done: > 1. LinkageError in method loadMainClass is handled separately and with > a new message in launcher.properties 2. Test case updated to print the error > message(even when test passes) and to print the final test result message as > suggested by Kumar. > The updated LauncherHelper looks okay although you can drop the catching of LinkageError at L641 because it will be caught by the outer catch at L647. -Alan.
RE: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized
Thank you Alan and Kumar for your review.
Here is the updated webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.03
Changes done:
1. LinkageError in method loadMainClass is handled separately and with a new
message in launcher.properties
2. Test case updated to print the error message(even when test passes) and to
print the final test result message as suggested by Kumar.
Regards,
Ramanand.
-Original Message-
From: Kumar Srinivasan
Sent: Wednesday, January 25, 2017 9:30 PM
To: Ramanand Patil <ramanand.pa...@oracle.com>
Cc: Alan Bateman <alan.bate...@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if
start-class cannot be initialized
116 if (result.isOK()) {
117 throw new Exception("Test Passed Unexpectedly!");
118 } else if (result.contains("JNI error")) {
119 result.testOutput.forEach(System.err::println);
120 throw new Exception("Test Failed with JNI error!");
121 }
+ 122 System.out.println("Test passes, failed with expected error message");
As a sanity I would add the above. This will make it clear to someone trying to
triage an error.
Kumar
On 1/25/2017 2:10 AM, Ramanand Patil wrote:
> Hi Kumar,
> Thank you for the review and suggestions for the test case.
> Here is the updated Webrev:
> http://cr.openjdk.java.net/~rpatil/8167063/webrev.02/
>
>
> Regards,
> Ramanand.
>
> -----Original Message-
> From: Kumar Srinivasan
> Sent: Tuesday, January 24, 2017 2:50 AM
> To: Ramanand Patil <ramanand.pa...@oracle.com>
> Cc: Alan Bateman <alan.bate...@oracle.com>; core-libs-dev
> <core-libs-dev@openjdk.java.net>
> Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred"
> if start-class cannot be initialized
>
>
> Hi Ramanand,
> test/tools/launcher/LauncherMessageTest.java
>
> 1)
>
> 116 String[] commands = {"java", "--module-path", modules.getPath(),
> 117 "-m", "mod.b/pkgB.ClassB"};
>
> The execution PATH may or may not contain the JAVA_HOME_UNDER_TEST/bin, so
> the right "java" may not be picked up, suggest using the TestHelper's
> javaCmd field that will be set correctly.
> Used TestHelper.javaCmd for instead of "java"
>
> 2)
>
>122 if (!result.isOK() && result.contains("JNI error")) {
>123 result.testOutput.forEach(System.err::println);
>124 throw new RuntimeException("Test Failed with JNI error!");
>125 } else {
>126 System.out.println("Test Passed...");
>127 }
>
> The problem with 122 if it the test returns back with non-zero code, it will
> still fail with "Test Failed with JNI error", I prefer it to be broken up and
> return back the right message ie. if the test returns with non-zero code say
> that.
> Changed the way test results were checked to include the check for unexpected
> failure when result is not non-zero.(i.e. when isOK() returns true).
> 3)
> Also usage of RuntimeException is over the top, you can simply throw an
> Exception and have the main throws Exception, jtreg will do the needful.
> Changed. Used an Exception instead of RuntimeException
>
> 4)
>
>102 FileUtils.deleteFileWithRetry(Paths.get(modules.getPath() +
> File.separator + "mod.a.jar"));
>
> there are redundant use of File.separator, Paths.get should create the
> FS correctly on the target platform
> ex: Paths.get(modules.getPath(), "mod.a.jar");
>
> Personally I stay away from using raw File.separator, instead have the APIs
> do the work for me.
>
> -Removed all instances of File.separator
>
>
> Kumar
>
>
>> Hi Alan,
>> Thank you for the review.
>> My comments are inline and Webrev is updated here:
>> http://cr.openjdk.java.net/~rpatil/8167063/webrev.01/
>>
>> Change Summary:
>> - Removed SecurityException handling
>> - Updated the error message in launcher.properties
>> - Removed loadModuleMainClass0 method and moved the code back into
>> original loadModuleMainClass
>> - NoClassDefFoundError is replaced by its parent class LinkageError
>> in method loadMainClass
>>
>> Regards,
>> Ramanand.
>>
>> -Original Message-
>> From: Alan Bateman
>> Sent: Friday, January 20, 2017 8:03 PM
>> To: Ramanand Patil <ramanand.pa...@oracle.com>;
>> core-libs-dev@openjdk.java.net
>> Subject: Re: RFR: 8167063: spurious message "A J
RE: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized
Hi Kumar,
Thank you for the review and suggestions for the test case.
Here is the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8167063/webrev.02/
Regards,
Ramanand.
-Original Message-
From: Kumar Srinivasan
Sent: Tuesday, January 24, 2017 2:50 AM
To: Ramanand Patil <ramanand.pa...@oracle.com>
Cc: Alan Bateman <alan.bate...@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if
start-class cannot be initialized
Hi Ramanand,
test/tools/launcher/LauncherMessageTest.java
1)
116 String[] commands = {"java", "--module-path", modules.getPath(),
117 "-m", "mod.b/pkgB.ClassB"};
The execution PATH may or may not contain the JAVA_HOME_UNDER_TEST/bin, so the
right "java" may not be picked up, suggest using the TestHelper's javaCmd
field that will be set correctly.
Used TestHelper.javaCmd for instead of "java"
2)
122 if (!result.isOK() && result.contains("JNI error")) {
123 result.testOutput.forEach(System.err::println);
124 throw new RuntimeException("Test Failed with JNI error!");
125 } else {
126 System.out.println("Test Passed...");
127 }
The problem with 122 if it the test returns back with non-zero code, it will
still fail with "Test Failed with JNI error", I prefer it to be broken up and
return back the right message ie. if the test returns with non-zero code say
that.
Changed the way test results were checked to include the check for unexpected
failure when result is not non-zero.(i.e. when isOK() returns true).
3)
Also usage of RuntimeException is over the top, you can simply throw an
Exception and have the main throws Exception, jtreg will do the needful.
Changed. Used an Exception instead of RuntimeException
4)
102 FileUtils.deleteFileWithRetry(Paths.get(modules.getPath() +
File.separator + "mod.a.jar"));
there are redundant use of File.separator, Paths.get should create the FS
correctly on the target platform
ex: Paths.get(modules.getPath(), "mod.a.jar");
Personally I stay away from using raw File.separator, instead have the APIs do
the work for me.
-Removed all instances of File.separator
Kumar
> Hi Alan,
> Thank you for the review.
> My comments are inline and Webrev is updated here:
> http://cr.openjdk.java.net/~rpatil/8167063/webrev.01/
>
> Change Summary:
> - Removed SecurityException handling
> - Updated the error message in launcher.properties
> - Removed loadModuleMainClass0 method and moved the code back into
> original loadModuleMainClass
> - NoClassDefFoundError is replaced by its parent class LinkageError in
> method loadMainClass
>
> Regards,
> Ramanand.
>
> -Original Message-
> From: Alan Bateman
> Sent: Friday, January 20, 2017 8:03 PM
> To: Ramanand Patil <ramanand.pa...@oracle.com>;
> core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred"
> if start-class cannot be initialized
>
> On 20/01/2017 13:21, Ramanand Patil wrote:
>
>> Hi all,
>> Please review the following bug fix:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8167063
>> Webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.00/
>>
>> Handled the SecurityException and LinkageError which can be thrown from
>> Class.forName(...) method used in LauncherHelper.java and added
>> corresponding error messages to launcher.properties.
>> Though the reported bug is because of the LinkageError, Security exception
>> is also handled to be safe from calling Class.forName method.
>>
> I see this changes loadMainClass to abort with resources that are keyed on
> java.launcher.cls.error6 and java.launcher.cls.error7 but they don't appear
> in the launcher.properties file. Does this work? I would assume
> MissingResourceException is thrown.
> Thanks for pointing this. I missed to add it to launcher.properties. But now
> I have changed error6 to point to java.launcher.cls.error1 and removed
> security exception handling.
>
> For java.launcher.module.error3 and java.launcher.module.error4 then "link"
> is likely to confuse people. Sure, there may be linkage errors but there are
> many linkage errors and I think would be a lot clearer if the message was
> "Unable to load main class {0} from module {1}\n\{2}".
> Updated the message as suggested.
>
> It's not clear to me that having a different message for security exceptions
> make sense, esp. when the exception is printed. So I think I would drop that.
> Yes, I agree to drop out the Security exceptions handling.
&
RE: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized
Hi Alan,
Thank you for the review.
My comments are inline and Webrev is updated here:
http://cr.openjdk.java.net/~rpatil/8167063/webrev.01/
Change Summary:
- Removed SecurityException handling
- Updated the error message in launcher.properties
- Removed loadModuleMainClass0 method and moved the code back into original
loadModuleMainClass
- NoClassDefFoundError is replaced by its parent class LinkageError in method
loadMainClass
Regards,
Ramanand.
-Original Message-
From: Alan Bateman
Sent: Friday, January 20, 2017 8:03 PM
To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if
start-class cannot be initialized
On 20/01/2017 13:21, Ramanand Patil wrote:
> Hi all,
> Please review the following bug fix:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8167063
> Webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.00/
>
> Handled the SecurityException and LinkageError which can be thrown from
> Class.forName(...) method used in LauncherHelper.java and added corresponding
> error messages to launcher.properties.
> Though the reported bug is because of the LinkageError, Security exception is
> also handled to be safe from calling Class.forName method.
>
I see this changes loadMainClass to abort with resources that are keyed on
java.launcher.cls.error6 and java.launcher.cls.error7 but they don't appear in
the launcher.properties file. Does this work? I would assume
MissingResourceException is thrown.
Thanks for pointing this. I missed to add it to launcher.properties. But now I
have changed error6 to point to java.launcher.cls.error1 and removed security
exception handling.
For java.launcher.module.error3 and java.launcher.module.error4 then "link" is
likely to confuse people. Sure, there may be linkage errors but there are many
linkage errors and I think would be a lot clearer if the message was "Unable to
load main class {0} from module {1}\n\{2}".
Updated the message as suggested.
It's not clear to me that having a different message for security exceptions
make sense, esp. when the exception is printed. So I think I would drop that.
Yes, I agree to drop out the Security exceptions handling.
Also loadModuleMainClass0 is unusual method name, we've mostly (not
always) used the 0 suffix on native methods. In any case, it doesn't look like
it is needed, the code was okay in loadModuleMainClass as it was.
The method was added just to look the exception handling easier. I have removed
the extra method and placed the code back into loadModuleMainClass.
One final point is that is the nesting catching of LinkageError and
SecurityException in loadMainClass, I assume you don't need the inner catch.
I think both the catch blocks were needed, because the first(inner) catch was
already inside catch.(This was same as NoClassDefFoundError catching twice in
the same method). Now since NoClassDefFoundError is subclass of LinkageError, I
have replaced it with LinkageError. And I think still the same abort message
holds good. Please let me know if this is ok.
-Alan
RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized
Hi all, Please review the following bug fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8167063 Webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.00/ Handled the SecurityException and LinkageError which can be thrown from Class.forName(...) method used in LauncherHelper.java and added corresponding error messages to launcher.properties. Though the reported bug is because of the LinkageError, Security exception is also handled to be safe from calling Class.forName method. Regards, Ramanand.
RE: RFR: 8170316: (tz) Support tzdata2016j
Thank you Martin. Regards, Ramanand. From: Martin Buchholz [mailto:marti...@google.com] Sent: Tuesday, November 29, 2016 12:59 AM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: i18n-...@openjdk.java.net; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8169191: (tz) Support tzdata2016j Thanks as always for keeping the tzdata pipeline moving! Looks good to me. On Mon, Nov 28, 2016 at 1:24 AM, Ramanand Patil mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com> wrote: Hi all, Please review the latest TZDATA integration (tzdata2016j) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8170316 Webrev: http://cr.openjdk.java.net/~rpatil/8170316/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RE: RFR: 8170316: (tz) Support tzdata2016j
Hi Masayoshi, Sorry, that was an error from my side. Thank you for pointing that out and for your review. [Changed the BugID in Subject now]. Regards, Ramanand. -Original Message- From: Masayoshi Okutsu Sent: Wednesday, November 30, 2016 10:08 AM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: Martin Buchholz <marti...@google.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; i18n-...@openjdk.java.net Subject: Re: RFR: 8169191: (tz) Support tzdata2016j Sorry, but I was confused with the wrong bug ID in Subject... Looks good to me. Masayoshi On 11/29/2016 4:28 AM, Martin Buchholz wrote: > Thanks as always for keeping the tzdata pipeline moving! > Looks good to me. > > On Mon, Nov 28, 2016 at 1:24 AM, Ramanand Patil <ramanand.pa...@oracle.com> > wrote: > >> Hi all, >> Please review the latest TZDATA integration (tzdata2016j) to JDK9. >> Bug: https://bugs.openjdk.java.net/browse/JDK-8170316 >> Webrev: http://cr.openjdk.java.net/~rpatil/8170316/webrev.00/ >> >> All the TimeZone related tests are passed after integration. >> >> Regards, >> Ramanand. >>
RFR: 8169191: (tz) Support tzdata2016j
Hi all, Please review the latest TZDATA integration (tzdata2016j) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8170316 Webrev: http://cr.openjdk.java.net/~rpatil/8170316/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR: jdk8u-dev Backport of 8169191: (tz) Support tzdata2016i
Hi all, Please review the latest TZDATA integration (tzdata2016i) to JDK8U. Since tzdata is cumulative, this bug fix backports both the tzdata versions(tzdata2016h+tzdata2016i) into jdk8u. Bug: https://bugs.openjdk.java.net/browse/JDK-8169191 Webrev: http://cr.openjdk.java.net/~rpatil/8169537/webrev.00/ Jdk9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7f7091c1dd33 For reference: Jdk9 changeset for tzdata2016h integration(JDK-8168512): http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/3192d7aa428d All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR: 8169191: (tz) Support tzdata2016i
Hi all, Please review the latest TZDATA integration (tzdata2016i) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8169191 Webrev: http://cr.openjdk.java.net/~rpatil/8169191/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR: 8168512: (tz) Support tzdata2016h
Hi all, Please review the latest TZDATA integration (tzdata2016h) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8168512 Webrev: http://cr.openjdk.java.net/~rpatil/8168512/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RE: RFR: 8166875: (tz) Support tzdata2016g
Hi Masayoshi, Thank you for pointing that small miss. During testing phase, I had actually renamed "Asia/Rangoon" to "Asia/Yangon" (Asia/Rangoon entry was deleted) in TimeZoneNames*.java and this test case was failing along with other test cases, hence the bug ID tag was added there. But after correcting the TimeZoneNames*.java with correct changes I forgot to remove the bug ID from this bug. Here is the updated Webrev: http://cr.openjdk.java.net/~rpatil/8166875/webrev.02/ Regards, Ramanand. -Original Message- From: Masayoshi Okutsu Sent: Wednesday, October 05, 2016 8:49 AM To: Ramanand Patil <ramanand.pa...@oracle.com>; Martin Buchholz <marti...@google.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; i18n-...@openjdk.java.net Subject: Re: RFR: 8166875: (tz) Support tzdata2016g Hi Ramanand, I don't think it's appropriate to add the bug ID to test/sun/util/resources/cldr/Bug8134384.java. This test doesn't verify the TimeZoneNames*.java changes of this fix. Otherwise, the change looks OK to me. Thanks, Masayoshi On 10/4/2016 7:22 PM, Ramanand Patil wrote: > Hi Martin, > Thank you for your review and explanation of "Yangon". I liked the > translation "End of Strife". > > Looking at the description of the ZoneNames.java: > * The zid<->metazone mappings are based on CLDR metaZones.xml. > * The alias mappings are based on Link entries in tzdb data files. > > I had thought to not update this file because the CLDR metaZones.xml file > doesn’t have this entry updated. > But I think you are correct, since Link entry has this alias mentioned, there > is no harm in adding these entries to zidMap and aliasMap arrays. > Here is the updated Webrev: > http://cr.openjdk.java.net/~rpatil/8166875/webrev.01/ > > Changes done: > - Updated src/java.base/share/classes/java/time/format/ZoneName.java to > include "Yangon" entry. > - Removed unused imports from > src/java.base/share/classes/java/time/format/ZoneName.java > - Updated ZoneName.java in the test package as well to include > "Yangon". [test/java/time/test/java/time/format/ZoneName.java] > - Updated the bugID for > test/java/time/test/java/time/format/TestZoneTextPrinterParser.java since > this uses the "ZoneName.java" defined in test package. > > Also, looks like ZoneName.java is trying to maintain a comprehensive list of > zone names. Though I found very few zone names are missing from this file > like: "Europe/Busingen", "America/Fort_Nelson", "Antarctica/Troll" etc... > > > Regards, > Ramanand. > > From: Martin Buchholz [mailto:marti...@google.com] > Sent: Monday, October 03, 2016 8:55 PM > To: Ramanand Patil <ramanand.pa...@oracle.com> > Cc: i18n-...@openjdk.java.net; core-libs-dev > <core-libs-dev@openjdk.java.net> > Subject: Re: RFR: 8166875: (tz) Support tzdata2016g > > Hi Ramanand, > Pleased to meet you! > > I expected to see Yangon added to ZoneName, because of the existing > reference to Rangoon > > java/time/test/java/time/format/ZoneName.java:179:"Asia/Rangoon", > "Myanmar", "Asia/Rangoon", > > Is ZoneName.java trying to maintain a comprehensive list of zone names? > > """Yangon (ရန်ကုန်) is a combination of the two words yan (ရန်) and koun > (ကုန်), which mean "enemies" and "run out of", respectively. It is also > translated as "End of Strife".""" > > > On Mon, Oct 3, 2016 at 5:27 AM, Ramanand Patil > <mailto:ramanand.pa...@oracle.com> wrote: > HI all, > Please review the latest TZDATA integration (tzdata2016g) to JDK9. > Bug: https://bugs.openjdk.java.net/browse/JDK-8166875 > Webrev: http://cr.openjdk.java.net/~rpatil/8166875/webrev.00/ > > All the TimeZone related tests are passed after integration. > [BugID is updated for tests TimeZoneTest.java and Bug8134384.java, since they > verify the renamed TZID "Asia/Yangon"]. > > Regards, > Ramanand.
RE: RFR: 8166875: (tz) Support tzdata2016g
Hi Martin, Thank you for your review and explanation of "Yangon". I liked the translation "End of Strife". Looking at the description of the ZoneNames.java: * The zid<->metazone mappings are based on CLDR metaZones.xml. * The alias mappings are based on Link entries in tzdb data files. I had thought to not update this file because the CLDR metaZones.xml file doesn’t have this entry updated. But I think you are correct, since Link entry has this alias mentioned, there is no harm in adding these entries to zidMap and aliasMap arrays. Here is the updated Webrev: http://cr.openjdk.java.net/~rpatil/8166875/webrev.01/ Changes done: - Updated src/java.base/share/classes/java/time/format/ZoneName.java to include "Yangon" entry. - Removed unused imports from src/java.base/share/classes/java/time/format/ZoneName.java - Updated ZoneName.java in the test package as well to include "Yangon". [test/java/time/test/java/time/format/ZoneName.java] - Updated the bugID for test/java/time/test/java/time/format/TestZoneTextPrinterParser.java since this uses the "ZoneName.java" defined in test package. Also, looks like ZoneName.java is trying to maintain a comprehensive list of zone names. Though I found very few zone names are missing from this file like: "Europe/Busingen", "America/Fort_Nelson", "Antarctica/Troll" etc... Regards, Ramanand. From: Martin Buchholz [mailto:marti...@google.com] Sent: Monday, October 03, 2016 8:55 PM To: Ramanand Patil <ramanand.pa...@oracle.com> Cc: i18n-...@openjdk.java.net; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8166875: (tz) Support tzdata2016g Hi Ramanand, Pleased to meet you! I expected to see Yangon added to ZoneName, because of the existing reference to Rangoon java/time/test/java/time/format/ZoneName.java:179: "Asia/Rangoon", "Myanmar", "Asia/Rangoon", Is ZoneName.java trying to maintain a comprehensive list of zone names? """Yangon (ရန်ကုန်) is a combination of the two words yan (ရန်) and koun (ကုန်), which mean "enemies" and "run out of", respectively. It is also translated as "End of Strife".""" On Mon, Oct 3, 2016 at 5:27 AM, Ramanand Patil <mailto:ramanand.pa...@oracle.com> wrote: HI all, Please review the latest TZDATA integration (tzdata2016g) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8166875 Webrev: http://cr.openjdk.java.net/~rpatil/8166875/webrev.00/ All the TimeZone related tests are passed after integration. [BugID is updated for tests TimeZoneTest.java and Bug8134384.java, since they verify the renamed TZID "Asia/Yangon"]. Regards, Ramanand.
RFR: 8166875: (tz) Support tzdata2016g
HI all, Please review the latest TZDATA integration (tzdata2016g) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8166875 Webrev: http://cr.openjdk.java.net/~rpatil/8166875/webrev.00/ All the TimeZone related tests are passed after integration. [BugID is updated for tests TimeZoneTest.java and Bug8134384.java, since they verify the renamed TZID "Asia/Yangon"]. Regards, Ramanand.
RFR: 8160951, 8160958: "Test javax/xml/bind/marshal/8134111/UnmarshalTest.java should be added into :needs_jre group", "Test java/net/SetFactoryPermission/SetFactoryPermission.java should be added int
Hi all, Please review this trivial fix which addresses the mentioned 2 bugs. Bugs: 1. https://bugs.openjdk.java.net/browse/JDK-8160951 2. https://bugs.openjdk.java.net/browse/JDK-8160958 Webrev: http://cr.openjdk.java.net/~rpatil/8160951%2b8160958/webrev.00/ Only test/TEST.groups is modified to add these tests to the required group. Regards, Ramanand.
RE: RFR: 8161016: Strange behavior of URLConnection with proxy
Hi Aleksey,
Thank you for your review.
In the exception handler block: when last proxy fails, it was using a DIRECT
connection, but in the fixed version it was just a re-try once with the last
proxy before failing the connection.
Considering your points I think the alternate approach of not re-trying and
just failing after the last proxy is good. I have updated the code slightly
which does the same as you suggested.
And also updated the test case to use one more dummy proxy in MyProxySelector
class.
Here is the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8161016/webrev.01/
Regards,
Ramanand.
-Original Message-
From: Aleksey Shipilev [mailto:aleksey.shipi...@gmail.com]
Sent: Thursday, August 11, 2016 10:10 PM
To: Ramanand Patil; OpenJDK Network Dev list
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8161016: Strange behavior of URLConnection with proxy
On 08/11/2016 05:17 PM, Ramanand Patil wrote:
> Webrev: http://cr.openjdk.java.net/~rpatil/8161016/webrev.00/
>
> Fix: Instead of falling back to direct connection when last proxy
> fails to open connection, re-try once with the last proxy. An
> alternate solution can also be- don't try to open any connection when
> all set proxies fails to open a connection.
I wonder if the code should traverse the last proxy within the loop, not trying
to special-case it in the exception handler -- otherwise we would miss logging,
exceptions, and ProxySelector notifications coming from the last proxy?
E.g. instead of:
1116 } catch (IOException ioex) {
1117 if (p != Proxy.NO_PROXY) {
1118 sel.connectFailed(uri, p.address(), ioex);
1119 if (!it.hasNext()) {
1120 // re-try once with the last Proxy
1121 http = getNewHttpClient(url, p, connectTimeout, false);
1122 http.setReadTimeout(readTimeout);
1123 break;
1124 }
1125 } else {
1126 throw ioex;
1127 }
1128 continue;
1129 }
...do:
} catch (IOException ioex) {
if (p == Proxy.NO_PROXY) {
throw ioex;
}
sel.connectFailed(uri, p.address(), ioex);
if (it.hasNext()) {
continue; // try the next Proxy
} else {
throw ioex; // that was the last Proxy, time to fail
}
}
Thanks,
-Aleksey
RFR: 8161016: Strange behavior of URLConnection with proxy
Hi all, Please review the fix for Bug - https://bugs.openjdk.java.net/browse/JDK-8161016 Bug Description: When ProxySelector is present, i.e. there is minimum one proxy set (by System Property or System Default or using Custom ProxySelector implementation) connection should be opened through those set proxies and not through a DIRECT connection. Webrev: http://cr.openjdk.java.net/~rpatil/8161016/webrev.00/ Fix: Instead of falling back to direct connection when last proxy fails to open connection, re-try once with the last proxy. An alternate solution can also be- don't try to open any connection when all set proxies fails to open a connection. Regards, Ramanand.
RE: RFR: 8159684: (tz) Support tzdata2016f
Hi Masayoshi,
The reason for adding bugId(8159684) to TimeZoneTest.java was, it actually
tests the major change in the releases tzdata2016e and tzdata2016f.
tzdata2016e: "Africa/Cairo observes DST in 2016 from July 7 to the end of
October."
tzdata201f: The Egyptian government changed its mind on short notice, and
Africa/Cairo will not introduce DST starting 2016-07-07 after all.
After integrating, tzdata2016e, TimeZoneTest.java was failing because at line
no. 123 DST was false which should be true. (new ZoneDescriptor("ART", 120,
false))
And integrating tzdata2016f has again made the DST false in the test case.
Since the bug covers both the changes, I kept the bugID in the test case.
Please let me know if I still need to remove the bugID from test case.
Regards,
Ramanand.
-Original Message-
From: Masayoshi Okutsu
Sent: Wednesday, July 13, 2016 12:14 PM
To: Ramanand Patil; i18n-...@openjdk.java.net
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8159684: (tz) Support tzdata2016f
I don't think it's appropriate to add 8159684 to TimeZoneTest.java which
doesn't test the data changes of 2016e/f at all. I think there should be a time
zone data test in java.time to confirm the tzdata changes.
Otherwise, the changes look good to me.
Thanks,
Masayoshi
On 7/12/2016 6:27 PM, Ramanand Patil wrote:
> Hi all,
>
> Please review the latest TZDATA integration (tzdata2016f) to JDK9.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8159684
> Webrev: http://cr.openjdk.java.net/~rpatil/8159684/webrev.00/
>
> All the TimeZone related tests are passed after this integration.
>
> Regards,
> Ramanand.
RFR: 8159684: (tz) Support tzdata2016f
Hi all, Please review the latest TZDATA integration (tzdata2016f) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8159684 Webrev: http://cr.openjdk.java.net/~rpatil/8159684/webrev.00/ All the TimeZone related tests are passed after this integration. Regards, Ramanand.
RE: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access
Thank you Sean. Updated Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.03/ Regards, Ramanand. -Original Message- From: Seán Coffey Sent: Friday, June 17, 2016 9:34 PM To: Ramanand Patil; Daniel Fuchs; Bernd Eckenfels; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access Looks fine Ramanand. Please ensure the CCC is approved before pushing. 2 comments on the testcase : fileHendlers --> fileHandlers. Can you consider using the jdk.testlibrary.FileUtils.deleteFileIfExistsWithRetry for removal of directory that you create. It should be more stable in the long run. Regards, Sean. On 17/06/16 12:19, Ramanand Patil wrote: > Hi All, > > Gentle reminder... > > Regards, > Ramanand. > > -Original Message- > From: Ramanand Patil > Sent: Tuesday, June 14, 2016 9:51 PM > To: Daniel Fuchs; Bernd Eckenfels; core-libs-dev@openjdk.java.net > Subject: RE: RFR: 8153955: java.util.logging.FileHandler can not > create file synchronously over 101 access > > Hi all, > > May I request one more review for this bug fix.? > > Thank you very much Daniel for your review. > > > Regards, > Ramanand. > > -Original Message----- > From: Daniel Fuchs > Sent: Friday, June 10, 2016 7:47 PM > To: Ramanand Patil; Bernd Eckenfels; core-libs-dev@openjdk.java.net > Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not > create file synchronously over 101 access > > Hi Ramanand, > > Looks good to me now. > > best regards, > > -- daniel > > On 10/06/16 14:47, Ramanand Patil wrote: >> Hi Daniel and Bernd, >> Thank you for your reviews. >> Here is the updated Webrev: >> http://cr.openjdk.java.net/~rpatil/8153955/webrev.02/ >> >> Bernd, >> Considering the information that "if the FileHandler tries to open the >> filename and finds the file is currently in use by another process it will >> increment the unique number field and try again" what you suggested looks >> correct. But using concurrent/synchronous too should not make it wrong, >> because at this time all the files are used simultaneously and any file >> which is not in use will be opened and used by FileHandler. I think this >> should not make a big difference, but as per your suggestion I have altered >> only that section of logging.properties. >> Test is also updated with the suggested comment. Thank you. >> >> Daniel, >> 1. FileHandler.java: Changed the wordings as you suggested. >> 2. FileHandlerMaxLocksTest::main >> - Now using "user.dir" in createLoggerDir(). I saw some tests(like >> java/util/logging/FileHandlerPath.java) were already using user home or temp >> directory, so thought that should not be a problem. >> - Removed the WeekReference and using ArrayList for all the FileHandler >> instances. >> - Tested on Windows, Linux(Ubuntu14.04) and Solaris and test runs >> fine(able to delete all the log files and loggerDir). Hopefully, test will >> not have any issues on other platforms.(JPRT job is also passed). >> >> >> Regards, >> Ramanand. >> >> -Original Message- >> From: Bernd Eckenfels [mailto:e...@zusammenkunft.net] >> Sent: Thursday, June 09, 2016 11:16 PM >> To: Daniel Fuchs; core-libs-dev@openjdk.java.net >> Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not >> create file synchronously over 101 access >> >> Hello, >> >> I find the "concurrent/synchronous" comment a bit confusing. >> >> How about: >> >> # Number of attempts to obtain lock file in FileHandler # Implemented >> by incrementing the %u placeholder as documented # in FileHandler >> Javadoc >> >> In the test I would add a comment >> >> "200 raises the default limit of 100, we try 102 times" >> >> >> >> Gruss >> Bernd >> >> >> Am Thu, 9 Jun 2016 07:31:25 +0100 >> schrieb Daniel Fuchs <daniel.fu...@oracle.com>: >> >>> Hi Ramanand, >>> >>> Thanks for the updated. I still have some remarks: >>> >>> FileHandler.java: >>> >>> 98 *specifies the maximum number of concurrent locks hold >>> by 99 *FileHandler (defaults 100). >>> >>> Is the verb form correct: hold => held ? >>> >>> logging.properties: >>> >>> 42 # when the unique field %u is incremented as per the javadoc. >>> >>> "... as per the javadoc" => "... a
RE: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access
Hi All,
Gentle reminder...
Regards,
Ramanand.
-Original Message-
From: Ramanand Patil
Sent: Tuesday, June 14, 2016 9:51 PM
To: Daniel Fuchs; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: RE: RFR: 8153955: java.util.logging.FileHandler can not create file
synchronously over 101 access
Hi all,
May I request one more review for this bug fix.?
Thank you very much Daniel for your review.
Regards,
Ramanand.
-Original Message-
From: Daniel Fuchs
Sent: Friday, June 10, 2016 7:47 PM
To: Ramanand Patil; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file
synchronously over 101 access
Hi Ramanand,
Looks good to me now.
best regards,
-- daniel
On 10/06/16 14:47, Ramanand Patil wrote:
> Hi Daniel and Bernd,
> Thank you for your reviews.
> Here is the updated Webrev:
> http://cr.openjdk.java.net/~rpatil/8153955/webrev.02/
>
> Bernd,
> Considering the information that "if the FileHandler tries to open the
> filename and finds the file is currently in use by another process it will
> increment the unique number field and try again" what you suggested looks
> correct. But using concurrent/synchronous too should not make it wrong,
> because at this time all the files are used simultaneously and any file which
> is not in use will be opened and used by FileHandler. I think this should not
> make a big difference, but as per your suggestion I have altered only that
> section of logging.properties.
> Test is also updated with the suggested comment. Thank you.
>
> Daniel,
> 1. FileHandler.java: Changed the wordings as you suggested.
> 2. FileHandlerMaxLocksTest::main
> - Now using "user.dir" in createLoggerDir(). I saw some tests(like
> java/util/logging/FileHandlerPath.java) were already using user home or temp
> directory, so thought that should not be a problem.
> - Removed the WeekReference and using ArrayList for all the FileHandler
> instances.
> - Tested on Windows, Linux(Ubuntu14.04) and Solaris and test runs
> fine(able to delete all the log files and loggerDir). Hopefully, test will
> not have any issues on other platforms.(JPRT job is also passed).
>
>
> Regards,
> Ramanand.
>
> -Original Message-
> From: Bernd Eckenfels [mailto:e...@zusammenkunft.net]
> Sent: Thursday, June 09, 2016 11:16 PM
> To: Daniel Fuchs; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not
> create file synchronously over 101 access
>
> Hello,
>
> I find the "concurrent/synchronous" comment a bit confusing.
>
> How about:
>
> # Number of attempts to obtain lock file in FileHandler # Implemented
> by incrementing the %u placeholder as documented # in FileHandler
> Javadoc
>
> In the test I would add a comment
>
> "200 raises the default limit of 100, we try 102 times"
>
>
>
> Gruss
> Bernd
>
>
> Am Thu, 9 Jun 2016 07:31:25 +0100
> schrieb Daniel Fuchs <daniel.fu...@oracle.com>:
>
>> Hi Ramanand,
>>
>> Thanks for the updated. I still have some remarks:
>>
>> FileHandler.java:
>>
>>98 *specifies the maximum number of concurrent locks hold
>> by 99 *FileHandler (defaults 100).
>>
>> Is the verb form correct: hold => held ?
>>
>> logging.properties:
>>
>>42 # when the unique field %u is incremented as per the javadoc.
>>
>> "... as per the javadoc" => "... as per FileHandler API documentation"
>>
>> FileHandlerMaxLocksTest.java:
>>
>> - FileHandlerMaxLocksTest::createLoggerDir():
>>
>>75 String tmpDir = System.getProperty("java.io.tmpdir");
>>76 if (tmpDir == null) {
>>77 tmpDir = System.getProperty("user.home");
>>78 }
>>79 File tmpOrHomeDir = new File(tmpDir);
>>80 File loggerDir = new File(tmpOrHomeDir, LOGGER_DIR);
>>
>> The preferred place for a test to create file is the scratch
>> directory that jtreg creates for the test. The scratch directory
>> location is available from the "user.dir" system property.
>>
>> I suggest changing the code above to:
>>
>> String userDir = System.getProperty("user.dir", ".");
>> File loggerDir = new File(userDir, LOGGER_DIR);
>>
>> - FileHandlerMaxLocksTest::main
>>
>> I am not sure what you try to achieve by using WeakReference there.
>> I would suggest to store all created FileHandler instances into a
>> list, which would allow you
RE: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access
Hi all,
May I request one more review for this bug fix.?
Thank you very much Daniel for your review.
Regards,
Ramanand.
-Original Message-
From: Daniel Fuchs
Sent: Friday, June 10, 2016 7:47 PM
To: Ramanand Patil; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file
synchronously over 101 access
Hi Ramanand,
Looks good to me now.
best regards,
-- daniel
On 10/06/16 14:47, Ramanand Patil wrote:
> Hi Daniel and Bernd,
> Thank you for your reviews.
> Here is the updated Webrev:
> http://cr.openjdk.java.net/~rpatil/8153955/webrev.02/
>
> Bernd,
> Considering the information that "if the FileHandler tries to open the
> filename and finds the file is currently in use by another process it will
> increment the unique number field and try again" what you suggested looks
> correct. But using concurrent/synchronous too should not make it wrong,
> because at this time all the files are used simultaneously and any file which
> is not in use will be opened and used by FileHandler. I think this should not
> make a big difference, but as per your suggestion I have altered only that
> section of logging.properties.
> Test is also updated with the suggested comment. Thank you.
>
> Daniel,
> 1. FileHandler.java: Changed the wordings as you suggested.
> 2. FileHandlerMaxLocksTest::main
> - Now using "user.dir" in createLoggerDir(). I saw some tests(like
> java/util/logging/FileHandlerPath.java) were already using user home or temp
> directory, so thought that should not be a problem.
> - Removed the WeekReference and using ArrayList for all the FileHandler
> instances.
> - Tested on Windows, Linux(Ubuntu14.04) and Solaris and test runs
> fine(able to delete all the log files and loggerDir). Hopefully, test will
> not have any issues on other platforms.(JPRT job is also passed).
>
>
> Regards,
> Ramanand.
>
> -Original Message-
> From: Bernd Eckenfels [mailto:e...@zusammenkunft.net]
> Sent: Thursday, June 09, 2016 11:16 PM
> To: Daniel Fuchs; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not
> create file synchronously over 101 access
>
> Hello,
>
> I find the "concurrent/synchronous" comment a bit confusing.
>
> How about:
>
> # Number of attempts to obtain lock file in FileHandler # Implemented
> by incrementing the %u placeholder as documented # in FileHandler
> Javadoc
>
> In the test I would add a comment
>
> "200 raises the default limit of 100, we try 102 times"
>
>
>
> Gruss
> Bernd
>
>
> Am Thu, 9 Jun 2016 07:31:25 +0100
> schrieb Daniel Fuchs <daniel.fu...@oracle.com>:
>
>> Hi Ramanand,
>>
>> Thanks for the updated. I still have some remarks:
>>
>> FileHandler.java:
>>
>>98 *specifies the maximum number of concurrent locks hold
>> by 99 *FileHandler (defaults 100).
>>
>> Is the verb form correct: hold => held ?
>>
>> logging.properties:
>>
>>42 # when the unique field %u is incremented as per the javadoc.
>>
>> "... as per the javadoc" => "... as per FileHandler API documentation"
>>
>> FileHandlerMaxLocksTest.java:
>>
>> - FileHandlerMaxLocksTest::createLoggerDir():
>>
>>75 String tmpDir = System.getProperty("java.io.tmpdir");
>>76 if (tmpDir == null) {
>>77 tmpDir = System.getProperty("user.home");
>>78 }
>>79 File tmpOrHomeDir = new File(tmpDir);
>>80 File loggerDir = new File(tmpOrHomeDir, LOGGER_DIR);
>>
>> The preferred place for a test to create file is the scratch
>> directory that jtreg creates for the test. The scratch directory
>> location is available from the "user.dir" system property.
>>
>> I suggest changing the code above to:
>>
>> String userDir = System.getProperty("user.dir", ".");
>> File loggerDir = new File(userDir, LOGGER_DIR);
>>
>> - FileHandlerMaxLocksTest::main
>>
>> I am not sure what you try to achieve by using WeakReference there.
>> I would suggest to store all created FileHandler instances into a
>> list, which would allow you to close them properly in the finally
>> clause just before deleting LOGGER_DIR.
>>
>> Different operating systems might handle lock files differently.
>> I am afraid that some operating system might not let you delete a
>> directory that contains a file which is still locked by the syst
RE: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access
Hi, Please review the updated Webrev at: http://cr.openjdk.java.net/~rpatil/8153955/webrev.01/ FileHander.java and logging.properties files are updated as per Daniel's suggestion. Regards, Ramanand. -Original Message- From: Daniel Fuchs Sent: Wednesday, June 08, 2016 6:57 PM To: Ramanand Patil; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access Hi Ramanand, Thanks for looking into this. 1. FileHander.java: 94 *handler-name.append 95 *specifies whether the FileHandler should append onto 96 *any existing files (defaults to false). 97 *java.util.FileHandler.maxLocks 98 *specifies the maximum number of concurrent locks hold by 99 *FileHandler (defaults 100). line 97: a) this is java.util.logging.FileHandler, not java.util.FileHandler. b) for consistency with the specification of other FileHandler properties I suggest to change this line to: 97 *handler-name.maxLock 2. logging.properties: 31 # Default number of locks FileHandler can obtain synchronously. 32 # This specifies maximum number of concurrent locks by FileHandler 33 # when the unique field %u is incremented as per the javadoc. 34 java.util.logging.FileHandler.maxLocks = 100 Please move lines 31-34 between lines 44 and 45: that's where they belong. best regards, -- daniel On 08/06/16 12:06, Ramanand Patil wrote: > Hi all, > > Please review the following bug fix: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8153955 > > Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.00/ > > Fix: A new configurable java property- > "java.util.logging.FileHandler.maxLocks" is added to the FileHandler which > can be set in the custom logger config file. The default value of the this > property will be mentioned in the default config file.(logging.properties). > > > > > > Regards, > > Ramanand. >
RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access
Hi all, Please review the following bug fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8153955 Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.00/ Fix: A new configurable java property- "java.util.logging.FileHandler.maxLocks" is added to the FileHandler which can be set in the custom logger config file. The default value of the this property will be mentioned in the default config file.(logging.properties). Regards, Ramanand.
RE: RFR: 8151876: (tz) Support tzdata2016d
Hi all,
As mentioned by point 4 in my first review thread, one test case is still
failing for which a separate JBS bug is created.(
https://bugs.openjdk.java.net/browse/JDK-8157792 ).
Below edit is needed for green tests and issue will be revisited via
JDK-8157792.
--- a/test/sun/util/calendar/zi/TestZoneInfo310.java
+++ b/test/sun/util/calendar/zi/TestZoneInfo310.java
@@ -164,6 +164,10 @@
}
for (String zid : zids_new) {
+ if (zid.equals("Asia/Oral") || zid.equals("Asia/Qyzylorda")) {
+ // JDK-8157792 tracking this issue
+ continue;
+ }
ZoneInfoOld zi = toZoneInfoOld(TimeZone.getTimeZone(zid));
ZoneInfoOld ziOLD = (ZoneInfoOld)ZoneInfoOld.getTimeZone(zid);
if (! zi.equalsTo(ziOLD)) {
Regards,
Ramanand.
From: Seán Coffey
Sent: Tuesday, May 31, 2016 3:05 PM
To: Masayoshi Okutsu; Ramanand Patil; i18n-...@openjdk.java.net;
core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8151876: (tz) Support tzdata2016d
Masayoshi,
I still think the test adds value. At minimum it identifies timezones which
don't have a localised string in the JRE provider.
We need to start another discussion about how best to represent timezone names
for newly added timezones. At the moment, short and long formats will be of
"GMT±hh:mm" format. I suggest we use the IANA timezone name for the long format
name in future (e.g. "Asia/Tomsk" instead of "GMT+06:00")
For the sake of getting this into the JDK code lines, I suggest we go ahead
with your suggestion to remove the test for now. I've also reviewed this
Ramanand. Your edits look fine (including test removal for now)
Regards,
Sean.
On 31/05/2016 07:06, Masayoshi Okutsu wrote:
The CheckDisplayNames.java change is different from what I suggested and
doesn't make sense. But we no longer need the test. I'd suggest
CheckDisplayNames.java be removed. Otherwise, the fix looks OK to me.
Masayoshi
On 5/31/2016 3:03 AM, Ramanand Patil wrote:
Hi Masayoshi and All,
Here is the updated Webrev: HYPERLINK
"http://cr.openjdk.java.net/%7Erpatil/8151876/webrev.01/"http://cr.openjdk.java.net/~rpatil/8151876/webrev.01/
As suggested by Masayoshi, I have kept the existing names unchanged.
Also, this patch contains extra test case fixes for
1. java/util/TimeZone/CheckDisplayNames.java
2. java/util/TimeZone/Bug8149452.java
The exclusion for the newly added TimeZones is added in these test cases where
the entries are not present in the resources and the Short/Long display names
fallback to the GMT±hh:mm format.
Regards,
Ramanand.
From: Masayoshi Okutsu
Sent: Saturday, May 28, 2016 10:58 AM
To: Seán Coffey; Ramanand Patil; HYPERLINK
"mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net; HYPERLINK
"mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8151876: (tz) Support tzdata2016d
CLDR locale data defines time zone names, like this (in en.xml):
Almaty Time
Almaty Standard
Time
Almaty Summer Time
The CLDR converter tool tries to fill in the missing short names from the
legacy TimeZoneNames data. Removing existing names causes some unexpected
behavior. I think JDK-8157814 is an example of the unexpected behavior. And the
suggested fix in JDK-8157814 looks to me like a workaround.
I still think the existing names should be kept unchanged for this fix.
Thanks,
Masayoshi
On 5/28/2016 12:04 AM, Seán Coffey wrote:
I guess there's a low risk of timezone not being identified if being parsed in
through a formatter. Isn't such an approach discouraged though ? short IDs were
already subject to change in tzdata releases. Ramanand found one issue by
removal of these resource strings (so far) and that's captured in JDK-8157814
There's a decision to be made about how to use the GMT±hh:mm format for new
releases given IANA's new short ID identifier mechanism. I think that could be
discussed as a separate issue. I'd like to see us follow a similar approach to
IANA and use GMT identifiers on new timezones and perhaps even consider using
the IANA long name format also for the getDisplayName(..) calls that can be
made. e.g. "Asia/Almaty" instead of "Alma-Ata Time"
Regards,
Sean.
On 27/05/16 15:24, Masayoshi Okutsu wrote:
This change seems to beyond my proposal that the "GMT±hh:mm" format is used for
*new* zones with the "±hh" format. But this change removes *existing* zones
which have changed to use the "±hh" format in tzdata. Can this cause any
compatibility issues?
And have we agre
RE: RFR: 8151876: (tz) Support tzdata2016d
Hi Masayoshi, Thank you, I will delete this test before pushing the patch. Regards, Ramanand. From: Masayoshi Okutsu Sent: Tuesday, May 31, 2016 11:37 AM To: Ramanand Patil; Seán Coffey; i18n-...@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8151876: (tz) Support tzdata2016d The CheckDisplayNames.java change is different from what I suggested and doesn't make sense. But we no longer need the test. I'd suggest CheckDisplayNames.java be removed. Otherwise, the fix looks OK to me. Masayoshi On 5/31/2016 3:03 AM, Ramanand Patil wrote: Hi Masayoshi and All, Here is the updated Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Erpatil/8151876/webrev.01/"http://cr.openjdk.java.net/~rpatil/8151876/webrev.01/ As suggested by Masayoshi, I have kept the existing names unchanged. Also, this patch contains extra test case fixes for 1. java/util/TimeZone/CheckDisplayNames.java 2. java/util/TimeZone/Bug8149452.java The exclusion for the newly added TimeZones is added in these test cases where the entries are not present in the resources and the Short/Long display names fallback to the GMT±hh:mm format. Regards, Ramanand. From: Masayoshi Okutsu Sent: Saturday, May 28, 2016 10:58 AM To: Seán Coffey; Ramanand Patil; HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net; HYPERLINK "mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net Subject: Re: RFR: 8151876: (tz) Support tzdata2016d CLDR locale data defines time zone names, like this (in en.xml): Almaty Time Almaty Standard Time Almaty Summer Time The CLDR converter tool tries to fill in the missing short names from the legacy TimeZoneNames data. Removing existing names causes some unexpected behavior. I think JDK-8157814 is an example of the unexpected behavior. And the suggested fix in JDK-8157814 looks to me like a workaround. I still think the existing names should be kept unchanged for this fix. Thanks, Masayoshi On 5/28/2016 12:04 AM, Seán Coffey wrote: I guess there's a low risk of timezone not being identified if being parsed in through a formatter. Isn't such an approach discouraged though ? short IDs were already subject to change in tzdata releases. Ramanand found one issue by removal of these resource strings (so far) and that's captured in JDK-8157814 There's a decision to be made about how to use the GMT±hh:mm format for new releases given IANA's new short ID identifier mechanism. I think that could be discussed as a separate issue. I'd like to see us follow a similar approach to IANA and use GMT identifiers on new timezones and perhaps even consider using the IANA long name format also for the getDisplayName(..) calls that can be made. e.g. "Asia/Almaty" instead of "Alma-Ata Time" Regards, Sean. On 27/05/16 15:24, Masayoshi Okutsu wrote: This change seems to beyond my proposal that the "GMT±hh:mm" format is used for *new* zones with the "±hh" format. But this change removes *existing* zones which have changed to use the "±hh" format in tzdata. Can this cause any compatibility issues? And have we agreed to use the "GMT±hh:mm" format? Thanks, Masayoshi On 5/27/2016 10:19 PM, Seán Coffey wrote: Looks fine to me Ramanand. the recent 2016d changes have introduced some boundary issues for JDK rule parsing and those issues can be followed up in separate issues like you say. Regards, Sean. On 26/05/16 14:22, Ramanand Patil wrote: HI all, Please review the latest TZDATA integration (tzdata2016d) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8151876 Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Erpatil/8151876/webrev.00/"http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/ Patch Contains: 1. IANA tzdata2016d integration into JDK. [It also includes tzdata2016b and tzdata2016c which was not integrated]. 2. "GMT[+ -]hh:mm" is used for formatting of the modified or newly added TimeZones in tzdata2016d. [This is done to accommodate the IANA's new system where the zones use numeric time zone abbreviations like "+04" instead of invented abbreviations like "ASTT".] 3. Test case: java/time/test/java/time/format/TestZoneTextPrinterParser.java is updated to include the failures because of GMT[+ -]hh:mm format names. 4. Few other failing tests: For few other failing tests, new linked bugs are created and will be addressed in a separate patch. Regards, Ramanand.
RFR: 8151876: (tz) Support tzdata2016d
HI all, Please review the latest TZDATA integration (tzdata2016d) to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8151876 Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/ Patch Contains: 1. IANA tzdata2016d integration into JDK. [It also includes tzdata2016b and tzdata2016c which was not integrated]. 2. "GMT[+ -]hh:mm" is used for formatting of the modified or newly added TimeZones in tzdata2016d. [This is done to accommodate the IANA's new system where the zones use numeric time zone abbreviations like "+04" instead of invented abbreviations like "ASTT".] 3. Test case: java/time/test/java/time/format/TestZoneTextPrinterParser.java is updated to include the failures because of GMT[+ -]hh:mm format names. 4. Few other failing tests: For few other failing tests, new linked bugs are created and will be addressed in a separate patch. Regards, Ramanand.
RE: RFR: 8151876: (tz) Support tzdata2016c
Hi Roger, I don't think this is covered in any alternate tests, because this Displayname format is newly introduced with tzdata2016b for the newly added TimeZones. Anyway, I will double check on this to see if any test already covers this scenario otherwise I will update or add a new test case to cover this. Regards, Ramanand. -Original Message- From: Roger Riggs Sent: Monday, April 11, 2016 8:26 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8151876: (tz) Support tzdata2016c Hi Ramanand, The change to the test.java.time.format.TestZoneTextPrinterParser.test_ParseText just eliminates the test. Is there an alternate test that the formatter is returning the correct value for the GMT+/- cases? Roger On 4/11/2016 6:59 AM, Ramanand Patil wrote: > Hi all, > > I would like someone from java.time to do a second review for this. > > Regards, > Ramanand. > > -Original Message- > From: Masayoshi Okutsu > Sent: Tuesday, April 05, 2016 5:09 AM > To: Ramanand Patil; i18n-...@openjdk.java.net > Cc: core-libs-dev@openjdk.java.net > Subject: Re: RFR: 8151876: (tz) Support tzdata2016c > > Looks good to me. But I'd like someone from java.time to review the changes > to see if it's OK for java.time. > > Masayoshi > > On 4/4/2016 6:50 PM, Ramanand Patil wrote: >> Hi all, >> >> Please review the latest TZDATA (tzdata2016c) integration to JDK9. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8151876 >> >> Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/ >> >> All the TimeZone related tests are passed after integration. >> >> >> >> Please note that this patch includes both tzdata2016b and tzdata2016c >> integration. The tzdata2016b review was abandoned because tzdata2016c was >> already released. >> >> As suggested by Masayoshi, changes are made such that, "GMT+hh:mm" is used >> for formatting of the newly added TimeZones in tzdata2016b. >> >> [This is done to accommodate the IANA's new trial system where the >> new zones use numeric time zone abbreviations like "+04" instead of >> invented abbreviations like "ASTT".] >> >> >> >> Regards, >> >> Ramanand.
RE: RFR: 8151876: (tz) Support tzdata2016c
Hi all, I would like someone from java.time to do a second review for this. Regards, Ramanand. -Original Message- From: Masayoshi Okutsu Sent: Tuesday, April 05, 2016 5:09 AM To: Ramanand Patil; i18n-...@openjdk.java.net Cc: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8151876: (tz) Support tzdata2016c Looks good to me. But I'd like someone from java.time to review the changes to see if it's OK for java.time. Masayoshi On 4/4/2016 6:50 PM, Ramanand Patil wrote: > Hi all, > > Please review the latest TZDATA (tzdata2016c) integration to JDK9. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8151876 > > Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/ > > All the TimeZone related tests are passed after integration. > > > > Please note that this patch includes both tzdata2016b and tzdata2016c > integration. The tzdata2016b review was abandoned because tzdata2016c was > already released. > > As suggested by Masayoshi, changes are made such that, "GMT+hh:mm" is used > for formatting of the newly added TimeZones in tzdata2016b. > > [This is done to accommodate the IANA's new trial system where the new > zones use numeric time zone abbreviations like "+04" instead of > invented abbreviations like "ASTT".] > > > > Regards, > > Ramanand.
RFR: 8151876: (tz) Support tzdata2016c
Hi all, Please review the latest TZDATA (tzdata2016c) integration to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8151876 Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/ All the TimeZone related tests are passed after integration. Please note that this patch includes both tzdata2016b and tzdata2016c integration. The tzdata2016b review was abandoned because tzdata2016c was already released. As suggested by Masayoshi, changes are made such that, "GMT+hh:mm" is used for formatting of the newly added TimeZones in tzdata2016b. [This is done to accommodate the IANA's new trial system where the new zones use numeric time zone abbreviations like "+04" instead of invented abbreviations like "ASTT".] Regards, Ramanand.
RE: RFR: 8151876: (tz) Support tzdata2016b
Hi Masayoshi,
Thank you for clarification.
Since, the tzdata2016c is already released, I will update the this bug and send
a new review request to include both tzdata2016b and tzdata2016c.
Regards,
Ramanand.
-Original Message-
From: Masayoshi Okutsu
Sent: Monday, March 28, 2016 6:50 PM
To: Ramanand Patil; i18n-...@openjdk.java.net
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8151876: (tz) Support tzdata2016b
Hi Ramanand,
What I meant is to remove those new resources so that "GMT+hh:mm" is used for
formatting. There may be some tests that require changes.
Thanks,
Masayoshi
On 3/28/2016 7:31 PM, Ramanand Patil wrote:
> Hi Masayoshi,
>
> Currently I have not defined the new TimeZoneNames with +hh format, instead I
> have defined them as per the earlier convention like:
>
> +{"Asia/Barnaul", new String[] {"Barnaul Time", "BAT",
> + "Barnaul Summer Time", "BAST",
> + "Barnaul Time", "BAT"}},
>
> +{"Europe/Astrakhan", new String[] {"Astrakhan Time", "ASTT",
> + "Astrakhan Summer Time",
> "ASTST",
> + "Astrakhan Time",
> + "ASTT"}},
>
> +{"Europe/Ulyanovsk", new String[] {"Ulyanovsk Time", "ULT",
> + "Ulyanovsk Summer Time",
> "ULST",
> + "Ulyanovsk Time",
> + "ULT"}},
>
>
>
> Please let me know if this is fine.
>
>
> Regards,
> Ramanand.
>
> -Original Message-
> From: Masayoshi Okutsu
> Sent: Wednesday, March 23, 2016 7:22 PM
> To: Ramanand Patil; i18n-...@openjdk.java.net
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8151876: (tz) Support tzdata2016b
>
> Sorry for this belated response.
>
> I prefer to follow the tzdata abbreviations, like "+04". But that would
> require some major changes. An option would be not to define time zone names
> for the new zones with the +hh format.
>
> Thanks,
> Masayoshi
>
> On 3/17/2016 4:38 AM, Ramanand Patil wrote:
>> Hi all,
>>
>> Please review the latest TZDATA (tzdata2016b) integration to JDK9.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151876
>>
>> Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/
>>
>> All the TimeZone related tests are passed after integration.
>>
>>
>>
>> Please note that, as per the release notes: As a trial of a new system that
>> needs less information to be made up, the new zones use numeric time zone
>> abbreviations like "+04" instead of invented abbreviations like "ASTT".
>>
>> Since this is a trial system, I have ignored this in the current patch which
>> adds 3 new timezones. But I think it's worth discussing this new system
>> along with this bug and get the experts opinion on this.
>>
>> Do you think that JDK should also follow these IANA timezone naming system
>> for zone abbreviations in future ? Will it be convenient to use such
>> numeric time zone abbreviations ? (Also, it looks like the abbreviations
>> similar to "+03/+04" will be used for the zones linked to the rule set with
>> changing letters like: EST/EDT for US, MSK/MSD for RU, etc..).
>>
>>
>>
>> Regards,
>>
>> Ramanand.
>>
>>
RE: RFR: 8151876: (tz) Support tzdata2016b
Hi Masayoshi,
Currently I have not defined the new TimeZoneNames with +hh format, instead I
have defined them as per the earlier convention like:
+{"Asia/Barnaul", new String[] {"Barnaul Time", "BAT",
+ "Barnaul Summer Time", "BAST",
+ "Barnaul Time", "BAT"}},
+{"Europe/Astrakhan", new String[] {"Astrakhan Time", "ASTT",
+ "Astrakhan Summer Time",
"ASTST",
+ "Astrakhan Time", "ASTT"}},
+{"Europe/Ulyanovsk", new String[] {"Ulyanovsk Time", "ULT",
+ "Ulyanovsk Summer Time", "ULST",
+ "Ulyanovsk Time", "ULT"}},
Please let me know if this is fine.
Regards,
Ramanand.
-Original Message-
From: Masayoshi Okutsu
Sent: Wednesday, March 23, 2016 7:22 PM
To: Ramanand Patil; i18n-...@openjdk.java.net
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8151876: (tz) Support tzdata2016b
Sorry for this belated response.
I prefer to follow the tzdata abbreviations, like "+04". But that would require
some major changes. An option would be not to define time zone names for the
new zones with the +hh format.
Thanks,
Masayoshi
On 3/17/2016 4:38 AM, Ramanand Patil wrote:
> Hi all,
>
> Please review the latest TZDATA (tzdata2016b) integration to JDK9.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8151876
>
> Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/
>
> All the TimeZone related tests are passed after integration.
>
>
>
> Please note that, as per the release notes: As a trial of a new system that
> needs less information to be made up, the new zones use numeric time zone
> abbreviations like "+04" instead of invented abbreviations like "ASTT".
>
> Since this is a trial system, I have ignored this in the current patch which
> adds 3 new timezones. But I think it's worth discussing this new system along
> with this bug and get the experts opinion on this.
>
> Do you think that JDK should also follow these IANA timezone naming system
> for zone abbreviations in future ? Will it be convenient to use such numeric
> time zone abbreviations ? (Also, it looks like the abbreviations similar to
> "+03/+04" will be used for the zones linked to the rule set with changing
> letters like: EST/EDT for US, MSK/MSD for RU, etc..).
>
>
>
> Regards,
>
> Ramanand.
>
>
RFR: 8151876: (tz) Support tzdata2016b
Hi all, Please review the latest TZDATA (tzdata2016b) integration to JDK9. Bug: https://bugs.openjdk.java.net/browse/JDK-8151876 Webrev: http://cr.openjdk.java.net/~rpatil/8151876/webrev.00/ All the TimeZone related tests are passed after integration. Please note that, as per the release notes: As a trial of a new system that needs less information to be made up, the new zones use numeric time zone abbreviations like "+04" instead of invented abbreviations like "ASTT". Since this is a trial system, I have ignored this in the current patch which adds 3 new timezones. But I think it's worth discussing this new system along with this bug and get the experts opinion on this. Do you think that JDK should also follow these IANA timezone naming system for zone abbreviations in future ? Will it be convenient to use such numeric time zone abbreviations ? (Also, it looks like the abbreviations similar to "+03/+04" will be used for the zones linked to the rule set with changing letters like: EST/EDT for US, MSK/MSD for RU, etc..). Regards, Ramanand.
RE: RFR: JDK-8087104: DateFormatSymbols triggers this.clone() in the constructor
Hi all, May I request one more review for this bug? [Thank you Masayoshi for your review.] Regards, Ramanand. -Original Message- From: Masayoshi Okutsu Sent: Wednesday, February 24, 2016 1:46 PM To: Ramanand Patil; i18n-...@openjdk.java.net Cc: core-libs-dev@openjdk.java.net Subject: Re: RFR: JDK-8087104: DateFormatSymbols triggers this.clone() in the constructor Looks good to me. Masayoshi On 2/24/2016 4:40 PM, Ramanand Patil wrote: > Hi all, > Please review the fix for bug: > https://bugs.openjdk.java.net/browse/JDK-8087104 > Bug Description: DateFormatSymbols caches its own instance and calls > this.clone() in the constructor. Because of this, any subclass implementation > (which expects a field is always initialized to non-null in the constructor) > will throw NPE in its overridden clone() method while using any instance > variables which it assumed are initilaized in its contructor. > Webrev: http://cr.openjdk.java.net/~rpatil/8087104/webrev.00/ > Fix: Instead of using its own instance for caching and calling clone in > DateFormatSymbols, a nested class SymbolsCacheEntry is introduced. > > > Regards, > > Ramanand.
RFR: JDK-8087104: DateFormatSymbols triggers this.clone() in the constructor
Hi all, Please review the fix for bug: https://bugs.openjdk.java.net/browse/JDK-8087104 Bug Description: DateFormatSymbols caches its own instance and calls this.clone() in the constructor. Because of this, any subclass implementation (which expects a field is always initialized to non-null in the constructor) will throw NPE in its overridden clone() method while using any instance variables which it assumed are initilaized in its contructor. Webrev: http://cr.openjdk.java.net/~rpatil/8087104/webrev.00/ Fix: Instead of using its own instance for caching and calling clone in DateFormatSymbols, a nested class SymbolsCacheEntry is introduced. Regards, Ramanand.
RE:[PING] RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown error" instead of actual cause
Hi all, Please help me in getting this review done. Regards, Ramanand. -Original Message- From: Ramanand Patil Sent: Friday, February 05, 2016 10:46 PM To: core-libs-dev@openjdk.java.net Subject: RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown error" instead of actual cause Hi all Please review the fix for bug: https://bugs.openjdk.java.net/browse/JDK-8135259 Bug Description: Attempts to resolve a host unknown to the DNS server cause an UnknownHostException stating "unknown error" instead of "Name or service not known". Webrev: http://cr.openjdk.java.net/~rpatil/8135259/webrev.00/ Fix: Using a function call directly "gai_strerror(gai_error)" instead of using the function pointer which was declared but not initialized. Apart from this I have removed few other unused variables and function pointers. Brief description of how this bug was introduced is added to the bug comment. Regards, Ramanand.
RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown error" instead of actual cause
Hi all Please review the fix for bug: https://bugs.openjdk.java.net/browse/JDK-8135259 Bug Description: Attempts to resolve a host unknown to the DNS server cause an UnknownHostException stating "unknown error" instead of "Name or service not known". Webrev: http://cr.openjdk.java.net/~rpatil/8135259/webrev.00/ Fix: Using a function call directly "gai_strerror(gai_error)" instead of using the function pointer which was declared but not initialized. Apart from this I have removed few other unused variables and function pointers. Brief description of how this bug was introduced is added to the bug comment. Regards, Ramanand.
RE: RFR: 8148446: (tz) Support tzdata2016a
Hi Masayoshi/all,
Please review the updated Webrev at:
http://cr.openjdk.java.net/~rpatil/8148446/webrev.01/
Regards,
Ramanand.
-Original Message-
From: Masayoshi Okutsu
Sent: Thursday, February 04, 2016 12:17 PM
To: Ramanand Patil; i18n-...@openjdk.java.net
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8148446: (tz) Support tzdata2016a
Hi Ramanand,
I noticed that the zones in Yakutsk Time [1] seem to have their own names, such
as "Khandyga Time" for Asia/Khandyga, and you seem to follow that convention
for Asia/Chita. That causes some mismatch between the long names and
abbreviations.
I dag out some past tzdata fixes to see how that happened. What I found out is
that the 2013b fix [2] used "Yakutsk Time", but that the 2013c [3] fix used
"Khandyga Time". 2013b went to JDK 7 updates and earlier ones and 2013c went to
8. JDK 9 inherits the 8 fix.
I prefer to restore the 2013b convention for Asia/Yakutsk, Asia/Chita, and
Asia/Khandyga to have:
{"Yakutsk Time", "YAKT",
"Yakutsk Summer Time", "YAKST",
"Yakutsk Time", "YAKT"}
Thanks,
Masayoshi
[1] https://en.wikipedia.org/wiki/Yakutsk_Time
[2] http://hg.openjdk.java.net/jdk7u/jdk7u-dev/jdk/rev/b8009df64dc8
[3] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/ae35fdbab949
On 2/2/2016 8:00 PM, Ramanand Patil wrote:
> HI all,
>
> Please review the latest TZDATA integration (tzdata2016a) to JDK9.
>
> Bug - https://bugs.openjdk.java.net/browse/JDK-8148446
>
> Webrev - http://cr.openjdk.java.net/~rpatil/8148446/webrev.00/
>
>
>
> All the TimeZone related tests are passed after integration.
>
>
>
> Regards,
>
> Ramanand.
RFR: 8148446: (tz) Support tzdata2016a
HI all, Please review the latest TZDATA integration (tzdata2016a) to JDK9. Bug - https://bugs.openjdk.java.net/browse/JDK-8148446 Webrev - http://cr.openjdk.java.net/~rpatil/8148446/webrev.00/ All the TimeZone related tests are passed after integration. Regards, Ramanand.
RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer Exception While Compiling and building TZDB data file
HI all, Please review this trivial fix for Bug - https://bugs.openjdk.java.net/browse/JDK-8148570 Bug Description - While compiling and building TZDB data file(tzdata2016a) when transition rule(for Iran) doesn't have the day-of-week data a null pointer exception is thrown. Webrev - http://cr.openjdk.java.net/~rpatil/8148570/webrev.00/ Fix - While getting value from DayOfWeek, -1 is returned if the DayOfWeek is null. The reason to return "-1" being, the same value is checked later while getting day-of-week byte (dowbyte) at line no. 251, ZoneRules.java. Regards, Ramanand.
RE: RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer Exception While Compiling and building TZDB data file
Hi Roger, Writing the regression test will be little tricky, because the transition rules are read from the IANA provided tzdata files and during JDK building the tzdb.dat file is created. This issue was actually found after JDK9 build failure with integrated tzdata2016a files. Being a very obvious trivial bug fix, I feel this should be Ok without test. I have updated the label to include 'noreg-trivial'. Regards, Ramanand. -Original Message- From: Roger Riggs Sent: Friday, January 29, 2016 11:43 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer Exception While Compiling and building TZDB data file Hi Ramanand, Is there a specific regression test that can be written for the case? Otherwise, looks fine. Roger On 1/29/2016 12:56 PM, Ramanand Patil wrote: > HI all, > > Please review this trivial fix for Bug - > https://bugs.openjdk.java.net/browse/JDK-8148570 > > Bug Description - While compiling and building TZDB data file(tzdata2016a) > when transition rule(for Iran) doesn't have the day-of-week data a null > pointer exception is thrown. > > Webrev - http://cr.openjdk.java.net/~rpatil/8148570/webrev.00/ > > Fix - While getting value from DayOfWeek, -1 is returned if the DayOfWeek is > null. The reason to return "-1" being, the same value is checked later while > getting day-of-week byte (dowbyte) at line no. 251, ZoneRules.java. > > > > Regards, > > Ramanand. > > > >
RE: RFR: JDK-8147912: test "parseWithZoneWithoutOffset" of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on de_DE locale
Hi all, Please help me in reviewing this test fix. Regards, Ramanand. From: Ramanand Patil Sent: Monday, January 25, 2016 1:05 PM To: i18n-...@openjdk.java.net Cc: core-libs-dev@openjdk.java.net Subject: RFR: JDK-8147912: test "parseWithZoneWithoutOffset" of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on de_DE locale Hi all, Please review the trivial test bug fix for: https://bugs.openjdk.java.net/browse/JDK-8147912 Bug Description: Since the test case "parseWithZoneWithoutOffset" is using hard code as input data it will fail on some non-English locales (de_DE locale). Webrev: http://cr.openjdk.java.net/~rpatil/8147912/webrev.00 Fix: Even though hardcoded data is not preferred in compatibility test cases, this case was exception. English is provided as the default locale data for DateTimeFormatter in this testcase. Regards, Ramanand.
RE: RFR: JDK-8147912: test "parseWithZoneWithoutOffset" of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on de_DE locale
Hi Masayoshi, Thank you for review. This test was contributed by me for bug HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8066982"JDK-8066982. Running this only in English should be Ok as per me, since this was added just to test one of the parsing scenario when Zone information is available without Offset. Anyway, I will wait for java.time people to review this change. Regards, Ramanand. -Original Message- From: Masayoshi Okutsu Sent: Wednesday, January 27, 2016 11:23 AM To: Ramanand Patil; i18n-...@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: JDK-8147912: test "parseWithZoneWithoutOffset" of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on de_DE locale Looks OK to me. But I'd like some java.time people to review this change to see if the intention of this test is to run only in English. Thanks, Masayoshi On 1/27/2016 1:51 PM, Ramanand Patil wrote: > Hi all, > > > > Please help me in reviewing this test fix. > > > > Regards, > > Ramanand. > > From: Ramanand Patil > Sent: Monday, January 25, 2016 1:05 PM > To: HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net > Cc: HYPERLINK > "mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net > Subject: RFR: JDK-8147912: test "parseWithZoneWithoutOffset" > of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on > de_DE locale > > > > Hi all, > > Please review the trivial test bug fix for: > https://bugs.openjdk.java.net/browse/JDK-8147912 > > Bug Description: Since the test case "parseWithZoneWithoutOffset" is using > hard code as input data it will fail on some non-English locales (de_DE > locale). > > Webrev: http://cr.openjdk.java.net/~rpatil/8147912/webrev.00 > > Fix: Even though hardcoded data is not preferred in compatibility test cases, > this case was exception. English is provided as the default locale data for > DateTimeFormatter in this testcase. > > Regards, > Ramanand.
RFR: JDK-8147912: test "parseWithZoneWithoutOffset" of java/time/tck/java/time/format/TCKDTFParsedInstant.java fail on de_DE locale
Hi all, Please review the trivial test bug fix for: https://bugs.openjdk.java.net/browse/JDK-8147912 Bug Description: Since the test case "parseWithZoneWithoutOffset" is using hard code as input data it will fail on some non-English locales (de_DE locale). Webrev: http://cr.openjdk.java.net/~rpatil/8147912/webrev.00 Fix: Even though hardcoded data is not preferred in compatibility test cases, this case was exception. English is provided as the default locale data for DateTimeFormatter in this testcase. Regards, Ramanand.
RE: RFR: JDK-8144988: Unexpected timezone returned after parsing a date
Hi Masayoshi, Thank you for pointing that out. I have removed line 29 from the test. Please review the updated Webrev: http://cr.openjdk.java.net/~rpatil/8141243/webrev.01/ Regards, Ramanand. -Original Message- From: Masayoshi Okutsu Sent: Friday, January 15, 2016 8:18 AM To: Ramanand Patil; i18n-...@openjdk.java.net Cc: core-libs-dev@openjdk.java.net Subject: Re: RFR: JDK-8144988: Unexpected timezone returned after parsing a date Hi Ramanand, test/java/text/Format/DateFormat/Bug8141243.java: 28 * @run main Bug8141243 29 * @run main/othervm -Djava.locale.providers=COMPAT Bug8141243 "COMPAT" is a new name of "JRE" in JDK 9. It's not supported in 8u. I think COMPAT is slightly ignored and that it becomes the same thing as line 28. Line 29 should be removed. Otherwise, the fix looks OK to me. Masayoshi On 1/14/2016 9:21 PM, Ramanand Patil wrote: > Hi all, > > Please review the fix for bug: > https://bugs.openjdk.java.net/browse/JDK-8144988 > > Webrev: http://cr.openjdk.java.net/~rpatil/8141243/webrev.00 > > This is basically a backport of JDK9 bug: > https://bugs.openjdk.java.net/browse/JDK-8141243 > > JDK9 changeset(for reference): > http://hg.openjdk.java.net/jdk9/dev/jdk/rev/a1aa2671f281 > > Reason for the review request is because of: > i) Changes present in ResourceBundleGenerator.java file. > ii) The patch from JDK9 does not automatically apply as is after using > unshuffle_patch script. Few paths are adjusted as per the jdk8. > > Since CLDR became the default locale data in JDK9 leading incompatible > behavior with prior releases, the relevant code in ResourceBundleGenerator is > also backported in this patch. > Even though JDK8 has CLDR locale provider disabled by default, the code > change is done to be on safer side for cases where CLDR may be supporting > "UTC" ID in the future. > > > Regards, > Ramanand.
RFR: JDK-8144988: Unexpected timezone returned after parsing a date
Hi all, Please review the fix for bug: https://bugs.openjdk.java.net/browse/JDK-8144988 Webrev: http://cr.openjdk.java.net/~rpatil/8141243/webrev.00 This is basically a backport of JDK9 bug: https://bugs.openjdk.java.net/browse/JDK-8141243 JDK9 changeset(for reference): http://hg.openjdk.java.net/jdk9/dev/jdk/rev/a1aa2671f281 Reason for the review request is because of: i) Changes present in ResourceBundleGenerator.java file. ii) The patch from JDK9 does not automatically apply as is after using unshuffle_patch script. Few paths are adjusted as per the jdk8. Since CLDR became the default locale data in JDK9 leading incompatible behavior with prior releases, the relevant code in ResourceBundleGenerator is also backported in this patch. Even though JDK8 has CLDR locale provider disabled by default, the code change is done to be on safer side for cases where CLDR may be supporting "UTC" ID in the future. Regards, Ramanand.
RFR: 8145388: URLConnection.guessContentTypeFromStream returns image/jpg for some JPEG images
HI all, Please review this small fix for Bug - https://bugs.openjdk.java.net/browse/JDK-8145388 Bug Description - For some JPEG images the method java.net.URLConnection#guessContentTypeFromStream will return the MIME type image/jpg, which is not a valid, registered IANA mime type for JPEG images. Webrev - http://cr.openjdk.java.net/~ntv/ramanand/8145388/webrev.00/ Fix - Since "image/jpg" is not a valid Content Type, the method should return "image/jpeg" when APPn marker segment has 'EE' as a marker type. Regards, Ramanand.
RE: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition
Hi Roger,
The added import in DateTimeFormatter.java is because of the javadocs entry -
{@link ChronoLocalDateTime#atZone(ZoneId)}
Regards,
Ramanand.
From: Roger Riggs
Sent: Monday, December 14, 2015 8:36 PM
To: Ramanand Patil; core-libs-dev@openjdk.java.net
Cc: i18n-...@openjdk.java.net
Subject: Re: Review request for JDK-8066982: ZonedDateTime.parse()
returns wrong ZoneOffset around DST fall transition
Hi Ramanand,
Thanks for the cleanup of the test.
On 12/14/2015 3:14 AM, Ramanand Patil wrote:
Hi Roger and all,
Please review the updated Webrev:
http://cr.openjdk.java.net/~ntv/ramanand/8066982/webrev.02/
DateTimeFormatter.java has an added import that is unused and should be removed.
Looks fine.
Thanks, Roger
Bug: https://bugs.openjdk.java.net/browse/JDK-8066982
Roger, please see my comments about new tests:
- All my test methods were earlier generic with main method and hence had
private static qualifier. I have changed them now to match and to be consistent
with the existing tests. Thank you for pointing this.
- I agree with you on this. Particularly when we have tests around DST we can’t
avoid timezone data.
- I have defined dataProvider for every method and reduced the test data for
cases where zone is not present(testWithoutZoneWithoutOffset() and
testWithOffsetWithoutZone()).
But for the other 2 cases where zone is present(testWithZoneWithOffset() and
testWithZoneWithoutOffset()), I feel most of the data cases are necessary and
some are required to be on safer side if in future the timezone rule changes.
Also, this bug fix mainly affects these cases.
I have created the dataProvider kepping in mind the below cases for 2 DST zones.
1. Time before overlap
2. Time during Overlap
3. Time after overlap
4. Valid Offsets for each of these times
5. Zero Offset for each time
6. Few Positive and negative invalid offsets for each time
Regards,
Ramanand.
-Original Message-
From: Roger Riggs
Sent: Saturday, December 12, 2015 1:37 AM
To: HYPERLINK
"mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net
Cc: HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net
Subject: Re: Review request for JDK-8066982: ZonedDateTime.parse()
returns wrong ZoneOffset around DST fall transition
Hi,
Stephen, can you confirm that the added text and test in DateTimeFormatter is
not a specification change?
Our processes have a bit more to do if it is a spec change and it would limit
the backport to JDK 8.
This bug fix will cause an existing TCK/JCK test to fail but that is considered
also a bug and is fixed.
test/java/time/tck/java/time/TCKZonedDateTime.java
Ramanand, some comments on the new test:
- The 'private' qualifier on the tests and data providers is not used in the
current tests and
is not consistently present in the new one.
Since it has little/no function, the tests would be a bit cleaner without
it.
- Typically, test that have data dependencies (such as the timezone
data) cannot be used for
compatibility to the specification, but the data is stable and it seems
unavoidable in this case.
- Are all of the data cases necessary to validate the specification?
Redundant cases extend the testing time without adding more confidence to
the quality.
Thanks, Roger
On 12/10/2015 11:00 AM, Stephen Colebourne wrote:
> I believe this is suitable for committing, thanks, other reviews welcome!
> Stephen
>
>
>
> On 10 December 2015 at 15:36, Ramanand Patil "mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com> wrote:
>> Hi all,
>>
>> Please review the updated webrev:
>> HYPERLINK
>> "http://cr.openjdk.java.net/%7Eaefimov/8066982/webrev.01/"http://cr.openjdk.java.net/~aefimov/8066982/webrev.01/
>>
>> I have modified the fix and test cases as per inputs given by Stephen. Also,
>> I have added the javadocs changes in this patch which were proposed in the
>> bug.
>>
>> Bug link is: https://bugs.openjdk.java.net/browse/JDK-8066982
>>
>>
>> Regards,
>> Ramanand.
>>
>> -Original Message-
>> From: Stephen Colebourne [mailto:scolebou...@joda.org]
>> Sent: Wednesday, December 09, 2015 4:46 PM
>> To: core-libs-dev
>> Cc: i18n-dev
>> Subject: Re: Review request for JDK-8066982:
>> ZonedDateTime.parse() returns wrong ZoneOffset around DST fall
>> transition
>>
>> The logic looks fine.
>>
>> In the main code, this part
>> .getLong(INSTANT_SECONDS);
>> can be replaced with
>> .toEpochSecond();
>> which will be slightly faster.
>>
>> In the test case, this part
>> .plus(15, ChronoUnit.MINUTES);
>> can be replaced with
>> .plusMi
RE: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition
Hi Roger and all, Please review the updated Webrev: http://cr.openjdk.java.net/~ntv/ramanand/8066982/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8066982 Roger, please see my comments about new tests: - All my test methods were earlier generic with main method and hence had private static qualifier. I have changed them now to match and to be consistent with the existing tests. Thank you for pointing this. - I agree with you on this. Particularly when we have tests around DST we can’t avoid timezone data. - I have defined dataProvider for every method and reduced the test data for cases where zone is not present(testWithoutZoneWithoutOffset() and testWithOffsetWithoutZone()). But for the other 2 cases where zone is present(testWithZoneWithOffset() and testWithZoneWithoutOffset()), I feel most of the data cases are necessary and some are required to be on safer side if in future the timezone rule changes. Also, this bug fix mainly affects these cases. I have created the dataProvider kepping in mind the below cases for 2 DST zones. 1. Time before overlap 2. Time during Overlap 3. Time after overlap 4. Valid Offsets for each of these times 5. Zero Offset for each time 6. Few Positive and negative invalid offsets for each time Regards, Ramanand. -Original Message- From: Roger Riggs Sent: Saturday, December 12, 2015 1:37 AM To: HYPERLINK "mailto:core-libs-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Cc: HYPERLINK "mailto:i18n-...@openjdk.java.net; i18n-...@openjdk.java.net Subject: Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition Hi, Stephen, can you confirm that the added text and test in DateTimeFormatter is not a specification change? Our processes have a bit more to do if it is a spec change and it would limit the backport to JDK 8. This bug fix will cause an existing TCK/JCK test to fail but that is considered also a bug and is fixed. test/java/time/tck/java/time/TCKZonedDateTime.java Ramanand, some comments on the new test: - The 'private' qualifier on the tests and data providers is not used in the current tests and is not consistently present in the new one. Since it has little/no function, the tests would be a bit cleaner without it. - Typically, test that have data dependencies (such as the timezone data) cannot be used for compatibility to the specification, but the data is stable and it seems unavoidable in this case. - Are all of the data cases necessary to validate the specification? Redundant cases extend the testing time without adding more confidence to the quality. Thanks, Roger On 12/10/2015 11:00 AM, Stephen Colebourne wrote: > I believe this is suitable for committing, thanks, other reviews welcome! > Stephen > > > > On 10 December 2015 at 15:36, Ramanand Patil < HYPERLINK > "mailto:ramanand.pa...@oracle.com; ramanand.pa...@oracle.com> wrote: >> Hi all, >> >> Please review the updated webrev: >> http://cr.openjdk.java.net/~aefimov/8066982/webrev.01/ >> >> I have modified the fix and test cases as per inputs given by Stephen. Also, >> I have added the javadocs changes in this patch which were proposed in the >> bug. >> >> Bug link is: https://bugs.openjdk.java.net/browse/JDK-8066982 >> >> >> Regards, >> Ramanand. >> >> -Original Message- >> From: Stephen Colebourne [mailto:scolebou...@joda.org] >> Sent: Wednesday, December 09, 2015 4:46 PM >> To: core-libs-dev >> Cc: i18n-dev >> Subject: Re: Review request for JDK-8066982: >> ZonedDateTime.parse() returns wrong ZoneOffset around DST fall >> transition >> >> The logic looks fine. >> >> In the main code, this part >>.getLong(INSTANT_SECONDS); >> can be replaced with >>.toEpochSecond(); >> which will be slightly faster. >> >> In the test case, this part >> .plus(15, ChronoUnit.MINUTES); >> can be replaced with >> .plusMinutes(15) >> >> And >> .with(ChronoField.OFFSET_SECONDS, >> ZoneOffset.of(offsetSamples[j]).getTotalSeconds()) >> can be replaced with >> .with(ZoneOffset.of(offsetSamples[j])) >> >> In addition to the looping tests, I'd like to see the examples from the bug >> report as test cases. Those tests would be simple to follow and explain, >> whereas the looping tests are a little hard to follow. >> >> thanks for fixing this >> Stephen >> >> >> >> On 9 December 2015 at 07:44, Ramanand Patil < HYPERLINK >> "mailto:ramanand.pa...@oracle.com; ramanand.pa...@oracle.com> wrote: >>> HI all, >>> >>> >>> >>> Please review a fix
RE: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition
Hi all, Please review the updated webrev: http://cr.openjdk.java.net/~aefimov/8066982/webrev.01/ I have modified the fix and test cases as per inputs given by Stephen. Also, I have added the javadocs changes in this patch which were proposed in the bug. Bug link is: https://bugs.openjdk.java.net/browse/JDK-8066982 Regards, Ramanand. -Original Message- From: Stephen Colebourne [mailto:scolebou...@joda.org] Sent: Wednesday, December 09, 2015 4:46 PM To: core-libs-dev Cc: i18n-dev Subject: Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition The logic looks fine. In the main code, this part .getLong(INSTANT_SECONDS); can be replaced with .toEpochSecond(); which will be slightly faster. In the test case, this part .plus(15, ChronoUnit.MINUTES); can be replaced with .plusMinutes(15) And .with(ChronoField.OFFSET_SECONDS, ZoneOffset.of(offsetSamples[j]).getTotalSeconds()) can be replaced with .with(ZoneOffset.of(offsetSamples[j])) In addition to the looping tests, I'd like to see the examples from the bug report as test cases. Those tests would be simple to follow and explain, whereas the looping tests are a little hard to follow. thanks for fixing this Stephen On 9 December 2015 at 07:44, Ramanand Patil <ramanand.pa...@oracle.com> wrote: > HI all, > > > > Please review a fix for Bug - HYPERLINK > "https://bugs.openjdk.java.net/browse/JDK-8066982"JDK-8066982 > > > > Bug - Parsing a string with ZonedDateTime.parse() that contains zone offset > and zone ID "Europe/Berlin" returns a wrong ZonedDateAndTime (different > offset). This error starts exactly at the transition time (included) and ends > one hour later (excluded). > > > > Webrev - http://cr.openjdk.java.net/~aefimov/8066982/webrev.00/ > > > > One existing test case in TCKZonedDateTime.java is also modified, because - > when offset is invalid the local time is changed to make the result valid. > > > > > > Regards, > > Ramanand.
Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition
HI all, Please review a fix for Bug - HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8066982"JDK-8066982 Bug - Parsing a string with ZonedDateTime.parse() that contains zone offset and zone ID "Europe/Berlin" returns a wrong ZonedDateAndTime (different offset). This error starts exactly at the transition time (included) and ends one hour later (excluded). Webrev - http://cr.openjdk.java.net/~aefimov/8066982/webrev.00/ One existing test case in TCKZonedDateTime.java is also modified, because - when offset is invalid the local time is changed to make the result valid. Regards, Ramanand.
RE: Code Review for JDK-8141243: Unexpected timezone returned after parsing a date
Hi Masayoshi,
Thank you for your feedback and suggestion.
As you said, I understood that There's no perfect solution on the issue because
the short display names(abbreviations) can't be unique.
I was trying to apply the workaround by considering the cases where the short
display names are same as the Zone IDs. Anyways if the issue is reproducible in
JDK 9 with the legacy time zone names I will dig more into it and try to find
the correct fix.
Sorry, if I am asking very simple question(I am new to this group/process and
want to understand correctly),
I didn't understand what is meant by your suggestion of moving UTC display
names in all resource bundles (all TimeZoneNames*.java under
sun.util.resources) up to the compatibility group.
Thank you for your help.
Regards,
Ramanand.
-Original Message-
From: Masayoshi Okutsu
Sent: Tuesday, November 17, 2015 2:24 PM
To: Ramanand Patil; i18n-dev; core-libs-dev; jdk8u-dev
Subject: Re: Code Review for JDK-8141243: Unexpected timezone
returned after parsing a date
Hi Ramanand,
I don't think this fix is correct. This change mixes up time zone IDs and
display names. SimpleDateFomat should parse only display names.
There's no perfect solution on the issue because the short display names
(abbreviations) can't be unique. So, I'd suggest that the UTC display names in
all resource bundles (all TimeZoneNames*.java under
sun.util.resources) move up to the compatibility group.
BTW, the symptom is reproducible in JDK 9 with the legacy time zone names (run
java with -Djava.locale.providers=COMPAT). So, the fix should be done in JDK 9
first.
Thanks,
Masayoshi
On 11/16/2015 10:38 PM, Ramanand Patil wrote:
> Hi all,
>
>
>
> Please review a fix for Bug Id - HYPERLINK
> "https://bugs.openjdk.java.net/browse/JDK-8141243"JDK-8141243.
>
> <https://bugs.openjdk.java.net/browse/JDK-8141243>
>
>
>
> Issue - When parsing the virtual timezone "UTC" with
> java.text.SimpleDateFormat, the timezone is set to the first timezone that
> matches an actual timezone in the UTC group, which is Antarctica/Troll. When
> comparing this timezone with the result of TimeZone.getTimeZone("UTC"), we
> fail.
>
>
>
> Webrev - http://cr.openjdk.java.net/~aefimov/8141243/webrev.00/
>
>
>
>
>
> Regards,
>
> Ramanand.
>
>
