Ensure snapshots can be committed after migration #37165
Conversation
|
Auto-backport failed due to merge conflict for this commit 2836ecc because the test is not disabled in main. This commit is removed from this PR. In addition, spot bug reported a false positive which is now excluded. Other than that there are no changes from the original PR. |
| <Match> | ||
| <!-- This is a false positive for property 'commitPosition' because it is updated with in an actor. So atomic updates are guaranteed. --> | ||
| <Class name="io.camunda.zeebe.broker.system.partitions.impl.AsyncSnapshotDirector"/> | ||
| <Bug pattern="AT_NONATOMIC_OPERATIONS_ON_SHARED_VARIABLE"/> | ||
| </Match> |
There was a problem hiding this comment.
❓ But the getter is executed from the Main thread right, so this is an issue then? As long updates are not atomic.
There was a problem hiding this comment.
The getter is only used for testing though.
There was a problem hiding this comment.
Yeah, right now. I feel this is dangerous. It especially needs a specific test setup, as you did with the Controlled actor. There is no guarantee that someone creates a test with a normal actor or different executor. Idk I feel this has a high chance of creating flaky tests.
There was a problem hiding this comment.
I think you can wrap it with actor.call to be safe and it should work with your test set up as well. But avoid potential future misuse.
There was a problem hiding this comment.
Changed the getter to return an actor future.
There was a problem hiding this comment.
Thanks @deepthidevaki sorry for the delay 🙇🏼
We probably want to adjust this on the other branches as well.
I will update them. |
Description
Forward port of #37064
Related issues
closes #37046