Repository navigation
RATIS-2399. Introduce separate locks for commitIndex and snapshotIndex in RaftLogBase - #1612
Open
Elia-Renzoni wants to merge 4 commits into
Open
Elia-Renzoni wants to merge 4 commits into
Elia-Renzoni wants to merge 4 commits into
Conversation
Signed-off-by: Elia Renzoni <elia.renzoni03@gmail.com>
Signed-off-by: Elia Renzoni <elia.renzoni03@gmail.com>
szetszwo
reviewed
Oct 3, 2026
Comment on lines
+156
to
+169
| try(AutoCloseableLock writeLock = writeLock()) { | ||
| try(AutoCloseableLock commitLock = writeLock(LockType.COMMIT_INDEX); | ||
| AutoCloseableLock snapshotLock = writeLock(LockType.SNAPSHOT_INDEX)) { |
Contributor
There was a problem hiding this comment.
@Elia-Renzoni , SNAPSHOT_INDEX is only acquired here together with COMMIT_INDEX. All the other cases acquire COMMIT_INDEX. Since it already has COMMIT_INDEX, acquiring SNAPSHOT_INDEX has no effect. No?
Contributor
Author
There was a problem hiding this comment.
Yes, you're right. I've pushed commit fac0204 with the fix. However, I've changed the original scope of the PR quite a bit I had previously added two separate locks, but since snapshotIndexLock was effectively unused, I've gone back to a single lock as before and used updateToMax to update the index atomically. What do you think of the latest changes?
Revert the separate COMMIT_INDEX/SNAPSHOT_INDEX locks: SNAPSHOT_INDEX was only ever acquired together with COMMIT_INDEX, so it had no effect. Instead, update snapshotIndex and commitIndex in updateSnapshotIndex with RaftLogIndex.updateToMax, so it no longer needs the log lock. To keep commitIndex monotonic against this lock-free writer, updateCommitIndex also uses updateToMax; it still holds the log lock to keep its checks on the flush index and entry term consistent with the log. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Elia Renzoni <elia.renzoni03@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Elia Renzoni <elia.renzoni03@gmail.com>
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
What changes were proposed in this pull request?
Replace the single lock for both snapshotIndex and commitIndex in RaftLogBase with two separate locks.
What is the link to the Apache JIRA
Jira Issue link:
https://issues.apache.org/jira/browse/RATIS-2399?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel
How was this patch tested?
This patch was tested via unit test