Avoid timer cycle with start time triggering multiple times #10054

lzgabel · 2022-08-10T12:11:36Z

Description

If a repeating timer with a start time is defined, the engine is stopped for some time during which the timer is supposed to trigger multiple times, and the engine is restarted, then it should trigger only once and reschedule the next timer in the future. Also, if a past start time is defined, it triggers after one interval period. If a future start time is defined that is expected sooner than, at, or after one interval period, it triggers at the start time.

Related issues

closes #9680

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

lzgabel · 2022-08-10T12:13:25Z

Hi @korthout. Please take a moment to check this out. 🙇

korthout · 2022-08-12T09:03:34Z

Thanks @lzgabel I'll need a bit more time to understand this change.

In the meantime, please provide a test case that shows the new behavior.

lzgabel · 2022-08-12T11:05:57Z

In the meantime, please provide a test case that shows the new behavior.

I've submitted test cases.

I'll need a bit more time to understand this change.

Hi @korthout. I'm very sorry for disrupting your work schedule. 🙇

❓ Since Philipp was the reviewer of the last PR #9418, it may be more appropriate for him to review this directly. WDYT?

cc @saig0.

korthout

Thanks for the additions @lzgabel ❤️

👍 I like your test. It made it much more clear what is happening.

❌ IMO, we need another test that verifies the behavior at the processing level.

Let me know if you have any questions on how to write that test.

bpmn-model/src/test/java/io/camunda/zeebe/model/bpmn/util/time/RepeatingIntervalTest.java

github-actions · 2022-08-17T10:19:19Z

Test Results

  848 files ±  0   848 suites ±0 1h 34m 43s ⏱️ - 3m 44s
6 344 tests - 75 6 333 ✔️ - 75 11 💤 ±0 0 ❌ ±0
6 528 runs - 75 6 517 ✔️ - 75 11 💤 ±0 0 ❌ ±0

Results for commit 2435a06. ± Comparison against base commit 7cf5ab6.

♻️ This comment has been updated with latest results.

korthout

Thanks @lzgabel

👍 I like the test, but I don't think the behavior is correct yet.

❌ Please have a look at my change request.

bpmn-model/src/main/java/io/camunda/zeebe/model/bpmn/util/time/Interval.java

engine/src/test/java/io/camunda/zeebe/engine/processing/timer/TimerStartEventTest.java

lzgabel · 2022-08-19T12:50:13Z

Hi @korthout. Please take a moment to check this out. 🙇

korthout

Thanks @lzgabel ❤️

❌ The timer is still scheduled immediately, which would lead to multiple triggers after downtime. Please have a look at my comments.

engine/src/test/java/io/camunda/zeebe/engine/processing/timer/TimerStartEventTest.java

lzgabel · 2022-08-19T17:01:29Z

Hi @korthout. Since I ran into a flaky test after the test, I've appended a commit. Please check this out. Sorry for the increased review workload. 🙇

korthout

Really nice work @lzgabel 👍

This implementation fixes the bug 🎉

❌ I have one change request for the test to make sure it verifies the behavior correctly.

💭 There is one small flaw of this implementation: This version doesn't trigger the timer immediately if the start time of the cycle is in the past during the deployment of the process model. Instead, it creates the timer to be due after one interval period. This isn't necessarily wrong but could be unexpected. We can create a separate issue for this. Previously, I mentioned we could add a validation against using time cycles with a start time in the past. But, now I no longer think we should do this because the timer still triggers after one interval period, so I think this is reasonable behavior for 90% of use cases.

engine/src/test/java/io/camunda/zeebe/engine/processing/timer/TimerStartEventTest.java

korthout · 2022-08-22T09:29:11Z

The PR description currently contains:

Also, if a past start time is defined, it should trigger immediately.

🔧 Please make it clear, that this is not the current behavior. Currently,

if a past start time is defined, it triggers after one interval period
if a future start time is defined that is expected sooner than, at, or after one interval period, it triggers at the start time

If a repeating timer with a start time is defined, the engine is stopped for some time during which the timer is supposed to trigger multiple times, and the engine is restarted, then it should trigger only once and reschedule the next timer in the future. Also, if a past start time is defined, it triggers after one interval period. If a future start time is defined that is expected sooner than, at, or after one interval period, it triggers at the start time.

Avoid timer cycle with start time triggering multiple times, if a repeating timer with a start time is defined, the start time should not be less than current time.

lzgabel · 2022-08-22T11:12:05Z

Hi @korthout. I've fixed review comments. Please check this out. 🙇

korthout

Well done @lzgabel and many thanks! ❤️

LGTM 👍

bors merge

zeebe-bors-camunda · 2022-08-22T12:47:06Z

Build succeeded:

korthout requested changes Aug 16, 2022

View reviewed changes

bpmn-model/src/test/java/io/camunda/zeebe/model/bpmn/util/time/RepeatingIntervalTest.java Show resolved Hide resolved

lzgabel force-pushed the 9680-fix-timer-cycle-interval branch from 883e81a to f9fbb19 Compare August 17, 2022 09:03

lzgabel force-pushed the 9680-fix-timer-cycle-interval branch 2 times, most recently from 5f7b8a5 to 5cafc47 Compare August 17, 2022 12:22

lzgabel requested a review from korthout August 17, 2022 13:00

korthout requested changes Aug 18, 2022

View reviewed changes

lzgabel force-pushed the 9680-fix-timer-cycle-interval branch from 5cafc47 to 3ce1342 Compare August 19, 2022 12:48

lzgabel requested a review from korthout August 19, 2022 12:49

korthout requested changes Aug 19, 2022

View reviewed changes

engine/src/test/java/io/camunda/zeebe/engine/processing/timer/TimerStartEventTest.java Outdated Show resolved Hide resolved

engine/src/test/java/io/camunda/zeebe/engine/processing/timer/TimerStartEventTest.java Outdated Show resolved Hide resolved

lzgabel requested a review from korthout August 19, 2022 15:43

lzgabel force-pushed the 9680-fix-timer-cycle-interval branch 2 times, most recently from 09d1f98 to 2435a06 Compare August 19, 2022 16:47

korthout requested changes Aug 22, 2022

View reviewed changes

lzgabel added 4 commits August 22, 2022 19:05

test: the start time should not be less than the current time

bb3ee83

test: avoid timer cycle with start time triggering multiple times

775a51c

test: fix flaky timer cycle with start time

f95cd72

Avoid timer cycle with start time triggering multiple times, if a repeating timer with a start time is defined, the start time should not be less than current time.

lzgabel force-pushed the 9680-fix-timer-cycle-interval branch from 2435a06 to f95cd72 Compare August 22, 2022 11:10

korthout approved these changes Aug 22, 2022

View reviewed changes

zeebe-bors-camunda bot merged commit 0588f34 into camunda:main Aug 22, 2022

lzgabel deleted the 9680-fix-timer-cycle-interval branch August 22, 2022 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid timer cycle with start time triggering multiple times #10054

Avoid timer cycle with start time triggering multiple times #10054

lzgabel commented Aug 10, 2022 •

edited

lzgabel commented Aug 10, 2022

korthout commented Aug 12, 2022

lzgabel commented Aug 12, 2022 •

edited

korthout left a comment

github-actions bot commented Aug 17, 2022 •

edited

korthout left a comment

lzgabel commented Aug 19, 2022

korthout left a comment

lzgabel commented Aug 19, 2022 •

edited

korthout left a comment

korthout commented Aug 22, 2022

lzgabel commented Aug 22, 2022

korthout left a comment

zeebe-bors-camunda bot commented Aug 22, 2022

Avoid timer cycle with start time triggering multiple times #10054

Avoid timer cycle with start time triggering multiple times #10054

Conversation

lzgabel commented Aug 10, 2022 • edited

Description

Related issues

Definition of Done

lzgabel commented Aug 10, 2022

korthout commented Aug 12, 2022

lzgabel commented Aug 12, 2022 • edited

korthout left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2022 • edited

Test Results

korthout left a comment

Choose a reason for hiding this comment

lzgabel commented Aug 19, 2022

korthout left a comment

Choose a reason for hiding this comment

lzgabel commented Aug 19, 2022 • edited

korthout left a comment

Choose a reason for hiding this comment

korthout commented Aug 22, 2022

lzgabel commented Aug 22, 2022

korthout left a comment

Choose a reason for hiding this comment

zeebe-bors-camunda bot commented Aug 22, 2022

lzgabel commented Aug 10, 2022 •

edited

lzgabel commented Aug 12, 2022 •

edited

github-actions bot commented Aug 17, 2022 •

edited

lzgabel commented Aug 19, 2022 •

edited