Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: quick fix for a timer bug #1232

Merged
merged 4 commits into from
Aug 27, 2024
Merged

fix: quick fix for a timer bug #1232

merged 4 commits into from
Aug 27, 2024

Conversation

koevskinikola
Copy link
Member

Description

Fixed-rate timers don't play nicely with backward time adjustments. For fixed rate timers, an execution is scheduled for ex. 5 seconds in the future. When that execution runs, it schedules the next one, etc. However, when traveling backwards in time, the next scheduled execution never triggers.

The issue needs to be fixed properly in Zeebe. A temporary workaround is to adjust the tests with a future date time.

Related issues

related to #1230

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

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

Documentation:

  • Javadoc has been written
  • The documentation is updated

remcowesterhoud and others added 3 commits August 26, 2024 16:46
Authorizations aren't supported in the next version. The record is bound to change so it's not worth adding a proper
logger for this value type right now. It likely isn't worth adding one in the future either, since ZPT is in the process
of being deprecated.
Instead of logging nothing we can use the toString method of the record
Fixed rate timers don't play nicely with backwards
time adjustments. For fixed rate timers, an execution
is scheduled for ex. 5 seconds in the future. When that
execution runs, it schedules the next one, etc. However,
when traveling backwards in time, the next scheduled
execution never triggers.

The issue needs to be fixed properly in Zeebe. A temporary
workaround is to adjust the tests with a future date time.
Copy link

github-actions bot commented Aug 27, 2024

Test Results

 50 files  + 46   50 suites  +46   1m 33s ⏱️ + 1m 33s
138 tests +126  138 ✅ +127  0 💤 ±0  0 ❌  - 1 
438 runs  +434  438 ✅ +438  0 💤 ±0  0 ❌  - 4 

Results for commit 14318b8. ± Comparison against base commit 747e0d3.

♻️ This comment has been updated with latest results.

Add empty logger for authorization records
@remcowesterhoud
Copy link
Contributor

remcowesterhoud commented Aug 27, 2024

I merged my branch to add the authorization valuetype to the logger into this branch to make sure the CI doesn't fail on this and we can actually merge it.

The only relevant change in this PR is the commit @koevskinikola made.

@koevskinikola koevskinikola added this pull request to the merge queue Aug 27, 2024
Merged via the queue into main with commit 848365f Aug 27, 2024
9 of 10 checks passed
@koevskinikola koevskinikola deleted the 1230-timer-test-fix branch August 27, 2024 12:13
@megglos
Copy link
Contributor

megglos commented Aug 28, 2024

@koevskinikola @remcowesterhoud the tests still fail on main and also failed on the merge queue 🤔 can you take another look? CC @nicpuppa

@nicpuppa
Copy link
Contributor

Since this failures are blocking the alpha release, I'm in favour of disable the failing tests on the alpha release branch. What's the impact from an user perspective ?

@remcowesterhoud @koevskinikola

@koevskinikola
Copy link
Member Author

@megglos I'm having another look right now. @nicpuppa is it okay if I timebox the investigation for 30mins? After that we can ignore the tests so that we can release ZPT.

From what Lena explained, the issue is more on the Zeebe side, i.e. how the process schedule service works (src) and we'll need more time to fix that.

@koevskinikola
Copy link
Member Author

It seems like the TimerTest is failing only on the Testcontainers extension "variant". I had some issues with my Docker setup so I didn't have time to dig too deeply, but the Testcontainers extension is using a ContainerizedEngine implementation, and we use the InMemoryEngine implementation of the engine in the in-memory extension. There might be some differences in these implementations on how we handle the time jumps which leads the Testcontainers variant to still fail.

@nicpuppa I'll disable the Testcontainers variant of the TimerTest so that the release can resume. I'll raise a PR soon, for some reason the JUnit 5 @Disabled annotation is not picked up and the test continues to run locally.

@nicpuppa
Copy link
Contributor

Thanks @koevskinikola, please open a PR from the release branch 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants