raft: paginate the unapplied config changes scan #32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pav-kv
commented
Mar 8, 2023
Contributor
Author
ahrtr
reviewed
Mar 9, 2023
tbg
reviewed
Jun 6, 2023
Contributor
tbg
left a comment
tbg
left a comment
There was a problem hiding this comment.
This change is what I expected, I'm a bit surprised to see the discussion on it. Perhaps the two of you can clue me in on what the discussion is about, I was mostly confused reading it, so am likely missing something.
Signed-off-by: Pavel Kalinnikov <[email protected]>
Signed-off-by: Pavel Kalinnikov <[email protected]>
Signed-off-by: Pavel Kalinnikov <[email protected]>
Signed-off-by: Pavel Kalinnikov <[email protected]>
Member
|
The PR looks good to me. Can you rebase this PR although there is no conflict? There are huge differences between this PR and the main branch. |
Contributor
Author
Contributor
Author
ahrtr
reviewed
Jun 7, 2023
ahrtr
approved these changes
Jun 7, 2023
Member
ahrtr
left a comment
ahrtr
left a comment
There was a problem hiding this comment.
LGTM
Nice improvement! thx @pavelkalinnikov
tbg
approved these changes
Jun 7, 2023
Signed-off-by: Pavel Kalinnikov <[email protected]>
pav-kv
added a commit
to pav-kv/cockroach
that referenced
this pull request
Jun 7, 2023
This includes etcd-io/raft#32 which fixes an out-of-memory scenario during election with large unapplied Raft logs. Part of cockroachdb#104402 Epic: none Release note (bug fix): Fixed a bug in upstream etcd-io/raft which could result in pulling unlimited amount of log into memory, and lead to out-of-memory situations. With the fix, the log scan has a limited memory footprint.
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
Jun 8, 2023
104483: go.mod: upgrade etcd-io/raft to 515b142 r=tbg a=pavelkalinnikov This includes etcd-io/raft#32 which fixes an out-of-memory scenario during election with large unapplied Raft logs. Part of #104402 Epic: none Release note (bug fix): Fixed a bug in upstream etcd-io/raft which could result in pulling unlimited amount of log into memory, and lead to out-of-memory situations. With the fix, the log scan has a limited memory footprint. Co-authored-by: Pavel Kalinnikov <[email protected]>
pav-kv
added a commit
to cockroachdb/etcd
that referenced
this pull request
Jun 15, 2023
This commit patches etcd-io/raft#32 which fixes an out-of-memory scenario during election with large unapplied Raft logs. The patch does apply and build cleanly, so there is a fixup commit on top. Commits included: 313e96ce raft: factor out unapplied config changes check a22dbb1e raft: don't scan unapplied entries if applied==committed 2cb76edb raft: inline numOfPendingConf() != 0 check d980ecd4 raft: paginate the unapplied config changes scan 1df76294 raft: add raftLog.scan method for paginated reads Signed-off-by: Pavel Kalinnikov <[email protected]>
pav-kv
added a commit
to cockroachdb/etcd
that referenced
this pull request
Jun 15, 2023
This commit fixes the build after porting etcd-io/raft#32. Signed-off-by: Pavel Kalinnikov <[email protected]>
pav-kv
added a commit
to cockroachdb/raft
that referenced
this pull request
Jun 15, 2023
…-read raft: paginate the unapplied config changes scan There is a potentially unlimited memory usage spike possible in raft.hup() which reads all unapplied committed entries in order to check that there are no unapplied config changes. This PR paginates this scan so that the spike is limited to MaxCommittedSizePerReady. It also terminates the scan early if a config change has been found. Signed-off-by: Pavel Kalinnikov <[email protected]>
pav-kv
added a commit
to pav-kv/cockroach
that referenced
this pull request
Jun 15, 2023
This dependency upgrade includes a patched PR 32 from etcd-io/raft which fixes an out-of-memory scenario during election with large unapplied Raft logs. Part of cockroachdb#104402 Epic: none Release note (bug fix): Fixed a bug in upstream etcd-io/raft which could result in pulling unlimited amount of log into memory, and lead to out-of-memory situations. With the fix, the log scan has a limited memory footprint. Release justification: fixing a bug after a customer escalation
overvenus
added a commit
to tikv/raft-rs
that referenced
this pull request
Oct 15, 2023
Fix potentially unlimited memory usage spike possible in raft.hup() which reads all unapplied committed entries in order to check that there are no unapplied config changes. This PR paginates this scan so that the spike is limited to MaxCommittedSizePerReady. It also terminates the scan early if a config change has been found. It is ported from etcd-io/raft#32. Signed-off-by: Neil Shen <[email protected]>
overvenus
added a commit
to tikv/raft-rs
that referenced
this pull request
Oct 17, 2023
Fix potentially unlimited memory usage spike possible in raft.hup() which reads all unapplied committed entries in order to check that there are no unapplied config changes. This PR paginates this scan so that the spike is limited to MaxCommittedSizePerReady. It also terminates the scan early if a config change has been found. It is ported from etcd-io/raft#32. Signed-off-by: Neil Shen <[email protected]>
tisonkun
added a commit
to tisonkun/mephisto-archive
that referenced
this pull request
Oct 18, 2023
Fix potentially unlimited memory usage spike possible in raft.hup() which reads all unapplied committed entries in order to check that there are no unapplied config changes. This PR paginates this scan so that the spike is limited to MaxCommittedSizePerReady. It also terminates the scan early if a config change has been found. It is ported from etcd-io/raft#32. It is ported from tikv/raft-rs#530. Signed-off-by: tison <[email protected]>
tisonkun
added a commit
to tisonkun/mephisto-archive
that referenced
this pull request
Oct 18, 2023
Fix potentially unlimited memory usage spike possible in raft.hup() which reads all unapplied committed entries in order to check that there are no unapplied config changes. This PR paginates this scan so that the spike is limited to MaxCommittedSizePerReady. It also terminates the scan early if a config change has been found. It is ported from etcd-io/raft#32. It is ported from tikv/raft-rs#530. Signed-off-by: tison <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a potentially unlimited memory usage spike possible in
raft.hup()which reads all unapplied committed entries in order to check that there are no unapplied config changes. This PR paginates this scan so that the spike is limited toMaxCommittedSizePerReady. It also terminates the scan early if a config change has been found.An example effect of this change can be seen below (memory usage in CockroachDB). Before, there was a large spike to 12 GiB:
After, the scan was smeared out, and the memory usage is stable around 1 GiB: