-
Notifications
You must be signed in to change notification settings - Fork 578
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
Prefer replicating events instead of snapshots #7957
Conversation
There are a couple of tests that I'll need to adjust because they are assuming that we'd replicate snapshots in most cases. |
@deepthidevaki Seems like there are still some unresolved test failures that I have to work on, sorry for the early request for review. |
@oleschoenburg Let me know if you need help with the failing tests. |
Tests seem to run through now (except a temporary issue for LGTM, not sure how to restart that). At least on my machine the relevant tests seem unusually flaky. One issue seems to be that during tests, filling a segment takes too long. I'd like to add a large payload to every message that is used to fill the segment to hopefully speed up this process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Please see my comment on the new "condition" and the test to reproduce the error. Othwerise it looks good, just have some optional comments.
atomix/cluster/src/main/java/io/atomix/raft/roles/LeaderAppender.java
Outdated
Show resolved
Hide resolved
atomix/cluster/src/main/java/io/atomix/raft/roles/LeaderAppender.java
Outdated
Show resolved
Hide resolved
atomix/cluster/src/test/java/io/atomix/raft/RaftReplicationTest.java
Outdated
Show resolved
Hide resolved
atomix/cluster/src/test/java/io/atomix/raft/RaftReplicationTest.java
Outdated
Show resolved
Hide resolved
atomix/cluster/src/test/java/io/atomix/raft/RaftReplicationTest.java
Outdated
Show resolved
Hide resolved
qa/integration-tests/src/test/java/io/camunda/zeebe/it/clustering/ClusteredSnapshotTest.java
Outdated
Show resolved
Hide resolved
qa/integration-tests/src/test/java/io/camunda/zeebe/it/clustering/RestoreTest.java
Show resolved
Hide resolved
atomix/cluster/src/test/java/io/atomix/raft/RaftReplicationTest.java
Outdated
Show resolved
Hide resolved
This enables us to perform snapshots on members without triggering log compaction. To achieve this we remove the RaftSnapshotListener and (optionally) run compaction synchronously.
… be preferred Adds a config value that indicates how much a follower must lag behind the leader before we prefer replicating a snapshot instead of events. The default value of 100 was chosen arbitrarily and might need readjustment.
4dc1c4d
to
b05dcb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! 🎉
Please see my comment before merging.
qa/integration-tests/src/test/java/io/camunda/zeebe/it/clustering/ClusteredSnapshotTest.java
Outdated
Show resolved
Hide resolved
Various tests assumed that we always replicate snapshots. Here we either remove tests that are no longer necessary or adjust tests such that snapshots are replicated, for example by triggering log compaction
b05dcb0
to
6af180e
Compare
I'll run some benchmarks before merging to confirm that the current threshold of 100 is a sensible value. |
Looking at the benchmarks it appears that this PR has no performance impact. With the current benchmark setup, some followers are lagging behind by >10k events which means that the current threshold of 100 is too low to prevent replicating snapshots. We can still find a better threshold or a different method to choose between replicating snapshots or events later. |
bors merge |
Thanks @oleschoenburg for running the benchmark. |
Build succeeded: |
@npepinpe are we planning to backport this? |
Would this fix or help fix any bugs? |
My current assumption is that it fixes #7955 but I'm still investigating. I started a new benchmark yesterday and will keep it running for a while. |
Then we can do that 👍 @oleschoenburg @deepthidevaki - how do you see these changes? How is the risk/value ratio of backporting this? It doesn't seem like too much at a quick glance so I'd be fine with backporting, but you probably have a better idea. If you think it's fine and worth it, then can one of you please just walk Ole through how to backport things? Thanks! |
I don't see any risk backporting this. If there is a chance this fixes the bug, we can backport it. |
I'm not sure about the risk, I'll defer to @deepthidevaki for that. As for "is it worth": I'd be positively surprised if this solves #7955 because in our (admittedly limited) benchmarks we weren't able to see any impact. |
Lets wait until next week, then I will check my benchmark again. :) |
We can scratch that from our list. It is still failing with one partition and this fix. http://34.77.165.228/d/I4lo7_EZk/zeebe?orgId=1&from=1634801065067&to=1634854108575&var-DS_PROMETHEUS=Prometheus&var-namespace=zell-chaos-cw42&var-pod=All&var-partition=All We can see again lot of install requests 🤷 I think the breaking of performance is also highly depended on the state size |
Description
This adds a threshold on number of events a follower can lag behind a leader to decide wether we should replicate events or a snapshot. This should save resources because replicating a low number of events should be more efficient than replicating a snapshot.
We can't always replicate events even if the lag is below the threshold because some events might not be available anymore due to log compaction.
Related issues
closes #7784
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/0.25
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation: