raft: count storage reads in MsgAppResp test #139907
Merged
Conversation
This file contains 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
|
This change is |
hakuuww
changed the title
hakuuww
force-pushed
the
characterizationTestForLeaderCommitTermStorageAccess
branch
from
6043734 to
b48db84
Compare
pav-kv
reviewed
Jan 27, 2025
There was a problem hiding this comment.
Looking good. Some style suggestions, and request to fix comments.
Also, "characterization test" in the commit name seems to be something else (from description, it sounds more like our datadriven tests). Consider using simpler language, e.g. raft: count storage reads in MsgAppResp test.
hakuuww
force-pushed
the
characterizationTestForLeaderCommitTermStorageAccess
branch
from
b48db84 to
27d717f
Compare
|
amended the commit |
pav-kv
approved these changes
Jan 28, 2025
pkg/raft/raft_test.go
Outdated
|
|
||
| // Follower 2 says term2, index5 is already replicated | ||
| // The leader responds with the updated commit index to follower 2 | ||
| // The leader sends entry{term:2, index:6} to follower 2 |
There was a problem hiding this comment.
I'm not sure the leader sends any entry here, only a MsgApp with an updated commit index. Everything should be already in-flight to this follower 2, so things won't be sent again for a while.
Please double-check and fix the comment.
There was a problem hiding this comment.
I investigated the the behavior of the test. In the test code the leader does send additional entries along with the updated commit index after receiving MsgAppResp. (Although it shouldn't send any)
Leader sends the following after receiving MsgAppResp for follower acking entry{index:4, term:2}
1->2 MsgApp Term:2 Log:2/4 Commit:4 Entries:[ 2/5 EntryNormal "" 2/6 EntryNormal "" ]
This occurs because the inflight object in the leader's tracker was not updated correctly to remember we sent out all messages when we initially broadcasted it. Because this is test code.
I will remove the comments saying we send additional entries after receiving MsgApp from a follower.
…der commits Currently, when the raft leader wants to commit an entry(advance commit index), it must check the term of the entry it wants to commit and ensure the term number is the same as the current leader term(term of itself). This may involve a storage access for the term number if the entry has been persisted on stable storage and no longer in the unstable log of the leader node. This change adds an assertion for this existing storage access for term by adding a scenario that triggers storage access in TestLeaderAppResp() and asserting storage has been called(logged by MemoryStorage.callStats.term) Jira issue: CRDB-45763 Fixes: cockroachdb#137826 Release note: None
hakuuww
force-pushed
the
characterizationTestForLeaderCommitTermStorageAccess
branch
from
27d717f to
73fd0a4
Compare
pav-kv
approved these changes
Jan 28, 2025
hakuuww
changed the title
|
TYFTR! bors r=pav-kv |
craig bot
pushed a commit
that referenced
this pull request
Jan 29, 2025
139609: raft: optimize storage access for term when leader tries to commit r=pav-kv a=hakuuww When the raft leader wants to commit an entry(advance commit index), it must check the term of the entry it wants to commit and ensure the term number is the same as the current leader term(term of itself). Previously, this process may involve a storage access for the term number if the entry has been persisted on stable storage and no longer in the unstable log of the leader node. In fact this above scenario happens quite often. Which makes an optimization improving leader commit latency fruitful. This PR introduces an optimization that utilizes an invariant that is logically equivalent to checking whether the want-to-be-committed entry has the same term as the leader doing the commit. This is done by keeping an entry index in leader node's memory that signifies the point in time where the current leader became leader. When the leader wants to commit an entry, the leader compares the want-to-be-committed entry's index(available in memory) with the newly added index in this PR to determine whether the term matches. Thus completely eliminating storage access on this code path. A corresponding unit test is also modified to account for the elimination of storage access when leader tries to commit. A previous related PR(merged): #139907 Jira issue: [CRDB-45763](https://cockroachlabs.atlassian.net/browse/CRDB-45763) Fixes: #137826 Release note (performance improvement): removed a potential storage read from raft commit pipeline. This reduces the worst-case KV write latency. Co-authored-by: Anthony Xu <[email protected]>
craig bot
pushed a commit
that referenced
this pull request
Jan 30, 2025
139609: raft: optimize storage access for term when leader tries to commit r=pav-kv a=hakuuww When the raft leader wants to commit an entry(advance commit index), it must check the term of the entry it wants to commit and ensure the term number is the same as the current leader term(term of itself). Previously, this process may involve a storage access for the term number if the entry has been persisted on stable storage and no longer in the unstable log of the leader node. In fact this above scenario happens quite often. Which makes an optimization improving leader commit latency fruitful. This PR introduces an optimization that utilizes an invariant that is logically equivalent to checking whether the want-to-be-committed entry has the same term as the leader doing the commit. This is done by keeping an entry index in leader node's memory that signifies the point in time where the current leader became leader. When the leader wants to commit an entry, the leader compares the want-to-be-committed entry's index(available in memory) with the newly added index in this PR to determine whether the term matches. Thus completely eliminating storage access on this code path. A corresponding unit test is also modified to account for the elimination of storage access when leader tries to commit. A previous related PR(merged): #139907 Jira issue: [CRDB-45763](https://cockroachlabs.atlassian.net/browse/CRDB-45763) Fixes: #137826 Release note (performance improvement): removed a potential storage read from raft commit pipeline. This reduces the worst-case KV write latency. Co-authored-by: Anthony Xu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
count storage reads in MsgAppResp test
Currently, when the raft leader wants to commit an entry(advance commit index), it must check the term of the entry it wants to commit and ensure the term number is the same as the current leader term(term of itself). This may involve a storage access for the term number if the entry has been persisted on stable storage and no longer in the unstable log of the leader node.
This change adds an assertion for this existing storage access for term by adding a scenario that triggers storage access in TestLeaderAppResp() and asserting storage has been called(logged by MemoryStorage.callStats.term).
This is a characterization test that prepares for a following change that will avoid storage access for term in leader commit process.
Jira issue: CRDB-45763
Epic: None
Release note: None