Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
On Tue, 7 Jun 2022 14:11:57 GMT, Gaurav Chaudhari wrote: >> Gaurav Chaudhari has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - 8285838: Cleanup of trailing whitespace >> - 8285838: Corrected month comparison check for TZ DST > > Appreciate the suggestion, I fixed up my test and simplified it as you said, > and checked against a few spawned LocalDate(s) to verify the functionality is > still good. @Deigue Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See [OpenJDK Developers’ Guide](https://openjdk.java.net/guide/#working-with-pull-requests) for more information. - PR: https://git.openjdk.java.net/jdk/pull/8661
Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
On Wed, 1 Jun 2022 13:32:36 GMT, Gaurav Chaudhari wrote: >> This fix ensures that when a lookup for a custom TZ code fails, and an >> attempt is made to find the GMT offset in order to get the current time, >> Daylight savings rules are applied correctly. > > Gaurav Chaudhari has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285838: Cleanup of trailing whitespace > - 8285838: Corrected month comparison check for TZ DST Appreciate the suggestion, I fixed up my test and simplified it as you said, and checked against a few spawned LocalDate(s) to verify the functionality is still good. - PR: https://git.openjdk.java.net/jdk/pull/8661
Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
On Thu, 2 Jun 2022 18:14:55 GMT, Gaurav Chaudhari wrote:
> Is the suggestion here to substitute the setting of the TZ environment
> variable, and simply getting a date based off this `SimpleTimeZone` , so as
> to bypass the process creation and just have it as a more simpler test?
No. The test process still has to be spawned with the TZ env variable set. The
suggested piece is supposed to replace the following `if` in `runTZTest()`
method:
Calendar calendar = Calendar.getInstance();
Date time = calendar.getTime();
// Add 1 since getMonth() starts from 0.
int month = time.getMonth() + 1;
ZonedDateTime date = ZonedDateTime.ofInstant(time.toInstant(),
ZoneId.systemDefault());
if ((month > Month.MARCH.getValue() && month <
Month.OCTOBER.getValue()) ||
(month == Month.MARCH.getValue() &&
date.isAfter(getLastSundayOfMonth(date))) ||
(month == Month.OCTOBER.getValue() &&
date.isBefore(getLastSundayOfMonth(date {
-
PR: https://git.openjdk.java.net/jdk/pull/8661
Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
On Thu, 2 Jun 2022 18:07:39 GMT, Naoto Sato wrote: >> Gaurav Chaudhari has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - 8285838: Cleanup of trailing whitespace >> - 8285838: Corrected month comparison check for TZ DST > > I tried several runs of CI tests and found no failures. Sorry for the false > alarm. BTW, now I've looked the test more closely, the test can be simplified > by using `SimpleTimeZone`, such as: > > new SimpleTimeZone(360, "MEZ-1MESZ", Calendar.MARCH, -1, > Calendar.SUNDAY, 0, Calendar.OCTOBER, -1, Calendar.SUNDAY, > 0).inDaylightTime(new Date()) > > This piece will check the current date is in DST, emulating > `TZ=MEZ-1MESZ,M3.5.0,M10.5.0` Hi @naotoj , Thanks for revisiting that and confirming it is all good. Initially the problem was brought to my attention through a third party client utilizing this particular TZ code, and that is where we got the basis for the need of the fix, where we are bypassing typical tzdb access to find named Timezones such as "Europe/Belgium" and trying to find the GMT offset and computing the time. Is the suggestion here to substitute the setting of the TZ environment variable, and simply getting a date based off this `SimpleTimeZone` , so as to bypass the process creation and just have it as a more simpler test? This sounds perfectly fine as long as SimpleTimeZone is have the zone ID setup exactly as the `MEZ-1MESZ,M3.5.0,M10.5.0` indicated. I can try this out ... just wanted to confirm with you how to make use of the above instantiation. - PR: https://git.openjdk.java.net/jdk/pull/8661
Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
On Wed, 1 Jun 2022 13:32:36 GMT, Gaurav Chaudhari wrote: >> This fix ensures that when a lookup for a custom TZ code fails, and an >> attempt is made to find the GMT offset in order to get the current time, >> Daylight savings rules are applied correctly. > > Gaurav Chaudhari has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285838: Cleanup of trailing whitespace > - 8285838: Corrected month comparison check for TZ DST I tried several runs of CI tests and found no failures. Sorry for the false alarm. BTW, now I've looked the test more closely, the test can be simplified by using `SimpleTimeZone`, such as: new SimpleTimeZone(360, "MEZ-1MESZ", Calendar.MARCH, -1, Calendar.SUNDAY, 0, Calendar.OCTOBER, -1, Calendar.SUNDAY, 0).inDaylightTime(new Date()) This piece will check the current date is in DST, emulating `TZ=MEZ-1MESZ,M3.5.0,M10.5.0` - PR: https://git.openjdk.java.net/jdk/pull/8661
Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
On Wed, 1 Jun 2022 13:32:36 GMT, Gaurav Chaudhari wrote: >> This fix ensures that when a lookup for a custom TZ code fails, and an >> attempt is made to find the GMT offset in order to get the current time, >> Daylight savings rules are applied correctly. > > Gaurav Chaudhari has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285838: Cleanup of trailing whitespace > - 8285838: Corrected month comparison check for TZ DST The machine has `/etc/localtime` pointing to `/usr/share/zoneinfo/GMT`, thus the system zone is `GMT`. However, for some reason, I cannot reproduce it on that machine any longer. Could be some inconsistency in my repo. Will test in our CI system and let you know the result later. - PR: https://git.openjdk.java.net/jdk/pull/8661
Re: RFR: 8285838: DST not applying properly with zone id offset set with TZ env variable [v6]
> This fix ensures that when a lookup for a custom TZ code fails, and an > attempt is made to find the GMT offset in order to get the current time, > Daylight savings rules are applied correctly. Gaurav Chaudhari has updated the pull request incrementally with two additional commits since the last revision: - 8285838: Cleanup of trailing whitespace - 8285838: Corrected month comparison check for TZ DST - Changes: - all: https://git.openjdk.java.net/jdk/pull/8661/files - new: https://git.openjdk.java.net/jdk/pull/8661/files/9e3b1bb4..87832663 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8661=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8661=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8661.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8661/head:pull/8661 PR: https://git.openjdk.java.net/jdk/pull/8661
