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

Improve the heuristic on when to send a snapshot or events to a follower #7784

Closed
deepthidevaki opened this issue Sep 8, 2021 · 6 comments · Fixed by #7957
Closed

Improve the heuristic on when to send a snapshot or events to a follower #7784

deepthidevaki opened this issue Sep 8, 2021 · 6 comments · Fixed by #7957
Assignees
Labels
area/performance Marks an issue as performance related kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0

Comments

@deepthidevaki
Copy link
Contributor

When the leader replicates to a follower, it can either send the next event to be replicated or a snapshot at a later index if one exists. If the follower is lagging behind many events it is more efficient to send a snapshot and skip all the missing events. However if the follower is only lagging behind by a few events, replicating snapshot has more overhead. Now we always send the snapshot if the snapshot exists. Now that the follower is building its own state, replicating snapshot is wasteful because the state build by the follower will be thrown away and it has to restart from the new snapshot.

This behavior was observed in one of our benchmark.
There are frequent snapshot replications:
image

Frequent re-installation of streamprocessor also leads to temporary increase in memory consumption:
image

Frequent replication of snapshot is not optimal if the snapshot is big. Hence we need a better heuristic on when to send a snapshot vs when to send the event even if a snapshot exists at a higher index. A simple strategy can be to send the events if the number of events until the snapshot index is less than a threshold, else send the snapshot. A more complex strategy could be based on the size of snapshot vs size of missing events.
This also means we shouldn't compact the logs immediately after taking a snapshot, but wait until the followers are caught up with either the snapshot or the events. For safety, compact after a specific timeout even if followers are not caught up.

@deepthidevaki deepthidevaki added kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog area/performance Marks an issue as performance related labels Sep 8, 2021
@npepinpe npepinpe added this to Planned in Zeebe Sep 9, 2021
@Zelldon
Copy link
Member

Zelldon commented Sep 13, 2021

I like the second idea, but I would prefer the first and simpler solution for now. If we see issues with that we can still improve and move to a more sophisticated solution I think

@lenaschoenburg lenaschoenburg self-assigned this Oct 5, 2021
@lenaschoenburg
Copy link
Member

I've started to look into this a bit and wanted to note down what I found out and also confirm with you that I'm looking in the right direction.

I think the starting point for replicating snapshots or events is in LeaderAppender.tryToReplicateSnapshot where we try to replicate a snapshot if possible and otherwise we replicate events. If I understand correctly, we currently always meet the conditions to replicate the snapshot. It should be possible to "just" add a new condition that checks (heuristically) if it would be beneficial to only replicate the events instead.

@deepthidevaki
Copy link
Contributor Author

You are in the right direction.

If I understand correctly, we currently always meet the conditions to replicate the snapshot.

If the follower is slower, then currently we "always" meet the condition. But it can also happen that the follower is expecting an event which is after the snapshot. In this case, the leader will always send the event and not the snapshot. This issue is regarding when the follower is slower, should we send the snapshot or the event.

It should be possible to "just" add a new condition that checks (heuristically) if it would be beneficial to only replicate the events instead.

Yes. You can add another condition to decide if you should send the snapshot or the event.

@deepthidevaki
Copy link
Contributor Author

Remember that it can happen that events are already deleted. So if the heuristics decide to send the event, it may not always have that the event. In that case it has to sent the snapshot.

@lenaschoenburg lenaschoenburg moved this from Planned to In progress in Zeebe Oct 6, 2021
@lenaschoenburg
Copy link
Member

lenaschoenburg commented Oct 7, 2021

I've tried to write a test that validates the new behaviour by disconnecting a follower, appending new entries and then reconnecting the follower. I've noticed that in the onInstall of the PassiveRole we always try to send a snapshot, regardless of how much the follower is lagging behind. Should we change this to use the new heuristic as well or should we leave it as is?

If we leave it as is I think I might need to write a new test helper that supports stopping/slowing down a follower so that I can create a follower that is lagging behind. (This seems related to #4586 which was just closed).

@deepthidevaki
Copy link
Contributor Author

onInstall deals with receiving snapshots. It doesn't trigger sending new snapshots. If you have issues with the test, it could be something else. Let's look into it together.

@npepinpe npepinpe moved this from In progress to Review in progress in Zeebe Oct 8, 2021
@ghost ghost closed this as completed in 8a4adb8 Oct 15, 2021
Zeebe automation moved this from Review in progress to Done Oct 15, 2021
@korthout korthout added the version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0 label Jan 4, 2022
@KerstinHebel KerstinHebel removed this from Done in Zeebe Mar 23, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Marks an issue as performance related kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants