(#34659) - Add Timezone Configuration to Watcher #117033

lukewhiting · 2024-11-19T15:23:06Z

This adds support for specifying a timezone as part of a Watcher schedule. It includes documentation, serialization and parsing changes.

Elements Of Note

Handling of DST discontinuity

Having time zones means we need to deal with transitions to and from DST where a given time may be skipped or duplicated during a transition (Known as a Discontinuity). See docs and CronTimezoneTests for expected behaviour during these events.

Changes to Cron Class' `getNextValidTimeAfter(...)` Method

To support timezones, it was necessary to update this method to be timezone aware. Unfortunately the underlying clock object (A legacy Calendar instance) behaved in an undesirable way during DST Discontinuity events causing watches to be skipped and duplicated.

A rewrite to LocalDateTime resolved such issues due to it's better handling of Local -> UTC time conversion and discontinuities however the getNextValidTimeAfter(...) algorithm depended heavily on quirks of the Calendar object such as being able to set the current hour to 24 causing the day to roll over and the hour to be set to 0.

A full rewrite of this method would risk introducing too many possible behaviour changes so instead I opted to wrap LocalDateTime and emulate the quirks of Calendar while retaining the better local date time -> UTC conversions, reducing the risk of unexpected behaviour changes and reducing implementation time at the expense of some additional code complexity.

Fixes #34659

github-actions · 2024-11-19T15:23:19Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-11-19T15:23:31Z

Pinging @elastic/es-docs (Team:Docs)

elasticsearchmachine · 2024-11-19T15:23:31Z

Pinging @elastic/es-data-management (Team:Data Management)

…schedules

The algorithm depends on some quirks of calendar but LocalDateTime correctly ignores DST during calculations so this uses a LocalDateTime with a wrapper to emulate some of Calendar's behaviours that the Cron algorithm depends on

…o LocalDateTime

dakrone

LGTM, I left a couple of minor doc comments and a question about breaking BWC with existing watches.

dakrone · 2024-11-19T18:21:45Z

docs/reference/watcher/trigger/schedule.asciidoc

+These occur when the clock moves forward, such as when daylight savings time starts
+and cause certain hours or minutes to be skipped. If your watch is scheduled to run
+during a gap transition, the watch is executed at the same time as before the transition.


Can you clarify if this also happens when the clock moves forward due to manual setting (e.g., if someone manually sets it 2 hours forward because it was incorrect)

No if someone (or NTP) moves the clock past a watch execution time, it will skip that execution

Sorry, what I meant was, could you add that clarification to the docs?

Ahhh right sure thing :-)

Change pushed

dakrone · 2024-11-19T18:22:16Z

docs/reference/watcher/trigger/schedule.asciidoc

+happen at 1:30AM (GMT+1).
+
+==== Overlap Transitions
+These occur when the clock moves backward, such as when daylight savings time ends


Same here about clarification for manual backward moves

In this case it would re-execute the watch when it hits the target time again

dakrone · 2024-11-21T18:08:14Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/scheduler/Cron.java

-    public static final int MAX_YEAR = Calendar.getInstance(UTC, Locale.ROOT).get(Calendar.YEAR) + 100;
+    // Restricted to 50 years as the tzdb only has correct DST transition information for countries using a lunar calendar
+    // for the next ~60 years
+    public static final int MAX_YEAR = Calendar.getInstance(UTC, Locale.ROOT).get(Calendar.YEAR) + 50;


Random question, but let's assume someone already had a cron expression in their Watcher configuration that was using a date 75 years in the future. Would changing this cause that Watch to fail now that it's a lower restriction?

No this is simply used as a short circuit for next runtime algorithm to stop it trying to calculate too far into the future. We recalculate next runtime on each watcher service start so eventually it would come in range and have its execution time calculated.

What could happen is if they had a watch 51 years in the future and never restarted the ES node for 50 years, then the watch would never run, but I think that cause should be uncommon enough that the protection this provides against long-running next runtime calculations / guarding against DST data being wrong is worth it.

…ictable watch execution

…tcher-timezone-support

dakrone

LGTM, left one more comment about a tiny typo. Thanks Luke, this has been requested for quite a while, so it's nice to be able to get it in!

docs/reference/watcher/trigger/schedule.asciidoc

Co-authored-by: Lee Hinman <[email protected]>

lukewhiting · 2024-11-22T16:41:24Z

@dakrone Yeah it was a bit of a mission and I would have liked time to completely rewrite the Cron class but I think this is a pragmatic solution in the end :-) I'll merge this Monday as I don't want to invoke the curse of the Friday evening merge.

elasticsearchmachine · 2024-11-25T09:52:41Z

💚 Backport successful

Status	Branch	Result
✅	8.x

* Add timezone support to Cron objects * Add timezone support to CronnableSchedule * XContent change to support parsing and display of TimeZone fields on schedules * Case insensitive timezone parsing * Doc changes * YAML REST tests * Equals, toString and HashCode now include timezone * Additional random testing for DST transitions * Migrate Cron class to use wrapped LocalDateTime The algorithm depends on some quirks of calendar but LocalDateTime correctly ignores DST during calculations so this uses a LocalDateTime with a wrapper to emulate some of Calendar's behaviours that the Cron algorithm depends on * Additional documentation to explain discontinuity event behaviour * Remove redundant conversions from ZoneId to TimeZone following move to LocalDateTime * Add documentation warning that manual clock changes will cause unpredictable watch execution * Update docs/reference/watcher/trigger/schedule.asciidoc Co-authored-by: Lee Hinman <[email protected]> --------- Co-authored-by: Lee Hinman <[email protected]>

* Add timezone support to Cron objects * Add timezone support to CronnableSchedule * XContent change to support parsing and display of TimeZone fields on schedules * Case insensitive timezone parsing * Doc changes * YAML REST tests * Equals, toString and HashCode now include timezone * Additional random testing for DST transitions * Migrate Cron class to use wrapped LocalDateTime The algorithm depends on some quirks of calendar but LocalDateTime correctly ignores DST during calculations so this uses a LocalDateTime with a wrapper to emulate some of Calendar's behaviours that the Cron algorithm depends on * Additional documentation to explain discontinuity event behaviour * Remove redundant conversions from ZoneId to TimeZone following move to LocalDateTime * Add documentation warning that manual clock changes will cause unpredictable watch execution * Update docs/reference/watcher/trigger/schedule.asciidoc --------- Co-authored-by: Lee Hinman <[email protected]>

* Add timezone support to Cron objects * Add timezone support to CronnableSchedule * XContent change to support parsing and display of TimeZone fields on schedules * Case insensitive timezone parsing * Doc changes * YAML REST tests * Equals, toString and HashCode now include timezone * Additional random testing for DST transitions * Migrate Cron class to use wrapped LocalDateTime The algorithm depends on some quirks of calendar but LocalDateTime correctly ignores DST during calculations so this uses a LocalDateTime with a wrapper to emulate some of Calendar's behaviours that the Cron algorithm depends on * Additional documentation to explain discontinuity event behaviour * Remove redundant conversions from ZoneId to TimeZone following move to LocalDateTime * Add documentation warning that manual clock changes will cause unpredictable watch execution * Update docs/reference/watcher/trigger/schedule.asciidoc Co-authored-by: Lee Hinman <[email protected]> --------- Co-authored-by: Lee Hinman <[email protected]>

lukewhiting added >enhancement >docs General docs changes :Data Management/Watcher v6.8.17 v9.0.0 labels Nov 19, 2024

elasticsearchmachine added Team:Data Management Meta label for data/management team Team:Docs Meta label for docs team labels Nov 19, 2024

lukewhiting requested review from dakrone and masseyke November 19, 2024 15:25

lukewhiting added 11 commits November 19, 2024 16:32

Add timezone support to Cron objects

ae7c608

Add timezone support to CronnableSchedule

69b442d

XContent change to support parsing and display of TimeZone fields on …

1fd6555

…schedules

Case insensitive timezone parsing

707ec04

Doc changes

e922e8d

YAML REST tests

56a0bd7

Equals, toString and HashCode now include timezone

0940d16

Additional random testing for DST transitions

3991e5e

Migrate Cron class to use wrapped LocalDateTime

612a2db

The algorithm depends on some quirks of calendar but LocalDateTime correctly ignores DST during calculations so this uses a LocalDateTime with a wrapper to emulate some of Calendar's behaviours that the Cron algorithm depends on

Additional documentation to explain discontinuity event behaviour

7a1cc72

Remove redundant conversions from ZoneId to TimeZone following move t…

e8c44bf

…o LocalDateTime

lukewhiting force-pushed the 34659-watcher-timezone-support branch from e0ceb34 to e8c44bf Compare November 19, 2024 16:32

lukewhiting added v8.17.0 and removed v6.8.17 labels Nov 20, 2024

elasticsearchmachine added v8.18.0 and removed v8.17.0 labels Nov 20, 2024

dakrone approved these changes Nov 21, 2024

View reviewed changes

Add documentation warning that manual clock changes will cause unpred…

7502b4f

…ictable watch execution

Merge branch 'main' of github.com:elastic/elasticsearch into 34659-wa…

7872643

…tcher-timezone-support

dakrone approved these changes Nov 22, 2024

View reviewed changes

docs/reference/watcher/trigger/schedule.asciidoc Outdated Show resolved Hide resolved

Update docs/reference/watcher/trigger/schedule.asciidoc

832e1ad

Co-authored-by: Lee Hinman <[email protected]>

lukewhiting merged commit 1d4c8d8 into elastic:main Nov 25, 2024
14 checks passed

lukewhiting added the auto-backport Automatically create backport pull requests when merged label Nov 25, 2024

lukewhiting mentioned this pull request Nov 25, 2024

[8.x] (#34659) - Add Timezone Configuration to Watcher (#117033) #117456

Merged

lukewhiting deleted the 34659-watcher-timezone-support branch November 25, 2024 09:55

lukewhiting restored the 34659-watcher-timezone-support branch November 25, 2024 09:55

pquentin mentioned this pull request Nov 27, 2024

Add support for new timezone field in watch schedules elastic/elasticsearch-specification#3181

Merged

lukewhiting deleted the 34659-watcher-timezone-support branch November 27, 2024 10:32

(#34659) - Add Timezone Configuration to Watcher #117033

(#34659) - Add Timezone Configuration to Watcher #117033

Uh oh!

Conversation

lukewhiting commented Nov 19, 2024

Elements Of Note

Handling of DST discontinuity

Changes to Cron Class' getNextValidTimeAfter(...) Method

Uh oh!

github-actions bot commented Nov 19, 2024

Uh oh!

elasticsearchmachine commented Nov 19, 2024

Uh oh!

elasticsearchmachine commented Nov 19, 2024

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukewhiting commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Nov 25, 2024

💚 Backport successful

Uh oh!

Uh oh!

Changes to Cron Class' `getNextValidTimeAfter(...)` Method

lukewhiting commented Nov 22, 2024 •

edited

Loading