fix: last committed index in BallotBox

[killme2008]

Motivation & Modification:

Main changes:

• Fixed Seems that readIndex() may return inconsistent value (e.g. 0) after node restart, is this a known issue? #1049
• Try to initialize the lastCommittedIndex in BallotBox to a proper value when restart Commit index may be smaller than snapshot index #1092
• Fail fast when log manager is overloaded while applying tasks Deadlock on configuration application in NodeImpl when disruptors are full #1105

Result:

#1049 , #1092 , #1105

Summary by CodeRabbit

• Refactor

  • Improved the initialization logic for BallotBox to enhance reliability.
  • Updated NodeImpl to handle log manager capacity before task execution.

• New Features

  • Added getLastSnapshotIndex() method to SnapshotExecutor.

• Tests

  • Enhanced test coverage for BallotBox and Node functionalities.
  • Added assertions and synchronization improvements in NodeTest.

• Bug Fixes

  • Handled IOException during directory creation in HybridLogStorage constructor.

[coderabbitai]

Walkthrough

The recent refactor in the jraft-core module includes consolidating BallotBox initialization into a dedicated initBallotBox() method, integrating it into the node start-up sequence. Additionally, several changes were made across the core, option, and storage classes to improve logging, testing, and error handling mechanisms, particularly addressing consistency issues on node restarts.

Changes

File Path	Change Summary
jraft-core/src/main/java/com/.../NodeImpl.java	Refactor BallotBox initialization logic; added new methods for testing and adjusted timeout handling.
jraft-core/src/main/java/com/.../BallotBoxOptions.java	Added lastCommittedIndex field with getter and setter methods.
jraft-core/src/main/java/com/.../SnapshotExecutor.java	Introduced getLastSnapshotIndex() method.
jraft-core/src/main/java/com/.../SnapshotExecutorImpl.java	Removed comments and annotations from getLastSnapshotIndex().
jraft-core/src/test/java/com/.../BallotBoxTest.java	Added tests for lastCommittedIndex initialization and consistency.
jraft-extension/java-log-storage-impl/src/main/java/com/.../HybridLogStorage.java	Added imports and exception handling for directory creation.
jraft-extension/java-log-storage-impl/src/test/java/com/.../NodeTest.java	Added assertions and synchronization improvements in test methods.

Sequence Diagram(s)

Suppressed due to the simplicity and varied nature of the changes.

Assessment against linked issues

Objective	Addressed	Explanation
Initialize lastCommittedIndex to avoid inconsistent readIndex values after a restart (issue #1049)	✅
Ensure readIndex() returns consistent values post node restart (issue #1049)	✅

Poem

The BallotBox has found its place,
In logs and snapshots, a stable base.
Through refactor's careful hand,
Consistency now rules the land.
A node restart, no fear to face,
For lastCommittedIndex holds the grace.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[killme2008]

@fengjiachun Please take a look at some fixes around lastCommittedIndex in BallotBox.

[coderabbitai]

Actionable comments posted: 6

Outside diff range and nitpick comments (2)

jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java (1)

33-33: Consider adding a brief comment describing the purpose of importing TimeoutException.

While the addition of TimeoutException is straightforward, a brief comment explaining its usage context in the test cases could enhance code readability and maintainability.

jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java (1)

1150-1153: Ensure the getBallotBox() method adheres to Java conventions.

The method getBallotBox() is marked with @OnlyForTest, which is typically used to indicate that a method should only be used for testing purposes. If this method is intended for broader use, consider removing this annotation to avoid confusion.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f45e961 and 5370279.

Files selected for processing (8)

• jraft-core/src/main/java/com/alipay/sofa/jraft/core/BallotBox.java (3 hunks)
• jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java (4 hunks)
• jraft-core/src/main/java/com/alipay/sofa/jraft/option/BallotBoxOptions.java (1 hunks)
• jraft-core/src/main/java/com/alipay/sofa/jraft/storage/SnapshotExecutor.java (1 hunks)
• jraft-core/src/main/java/com/alipay/sofa/jraft/storage/snapshot/SnapshotExecutorImpl.java (1 hunks)
• jraft-core/src/test/java/com/alipay/sofa/jraft/core/BallotBoxTest.java (2 hunks)
• jraft-extension/java-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/HybridLogStorage.java (2 hunks)
• jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java (4 hunks)

Files skipped from review due to trivial changes (1)

• jraft-core/src/main/java/com/alipay/sofa/jraft/storage/snapshot/SnapshotExecutorImpl.java

Additional comments not posted (4)

jraft-core/src/test/java/com/alipay/sofa/jraft/core/BallotBoxTest.java (1)

Line range hint 54-75: The changes to the test setup and the addition of the initWithLastCommittedIndex method are well-implemented and effectively test the initialization of lastCommittedIndex.

jraft-core/src/main/java/com/alipay/sofa/jraft/core/BallotBox.java (2)

Line range hint 169-185: The resetPendingIndex method includes appropriate checks and error handling to ensure state consistency during leadership transitions.

Verification successful

---

The BallotBox class and the resetPendingIndex method have been successfully identified in the codebase. The method implementation includes checks to ensure state consistency during leadership transitions, which are crucial for the correct functioning of the distributed system.

• The method verifies that pendingIndex is 0 and pendingMetaQueue is empty before proceeding.
• It also ensures that the newPendingIndex is greater than the lastCommittedIndex.

These checks are well-implemented and should handle the edge cases effectively.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify that the `resetPendingIndex` method handles edge cases correctly.

# Test: Check for the handling of edge cases in `resetPendingIndex`.
ast-grep --lang java --pattern $'class BallotBox {
  boolean resetPendingIndex(long $_) {
    $$$
  }
}'

Length of output: 105

---

---

Script:

#!/bin/bash
# Description: Verify the presence of the `BallotBox` class and the `resetPendingIndex` method.

# Step 1: Verify the presence of the `BallotBox` class.
ast-grep --lang java --pattern $'class BallotBox {
  $$$
}'

# Step 2: Verify the presence of the `resetPendingIndex` method within the `BallotBox` class.
ast-grep --lang java --pattern $'class BallotBox {
  public boolean resetPendingIndex(long $_) {
    $$$
  }
}'

# Step 3: Check for edge cases and error handling within the `resetPendingIndex` method.
rg 'resetPendingIndex' -A 20 -B 5 --glob '**/BallotBox.java'

Length of output: 4031

---

91-91: Initialization of lastCommittedIndex aligns with the objectives to handle node restarts correctly.

Verification successful

The lack of output from the ast-grep command indicates that the pattern did not match any implementation of getLastCommittedIndex in BallotBoxOptions. This could mean either the method does not exist, is named differently, or is located in a different class or file.

To proceed, we need to verify the existence and implementation of getLastCommittedIndex across the codebase. We will use rg to search for the method definition and ensure it is correctly implemented and used.

---

The rg command output confirms that the getLastCommittedIndex method is defined in multiple locations, including BallotBoxOptions.java. This verifies that the method is indeed implemented.

To ensure the method's functionality and its proper usage, we should check the implementation details of getLastCommittedIndex in BallotBoxOptions.java.

---

The getLastCommittedIndex method is implemented in BallotBoxOptions.java and correctly returns the lastCommittedIndex value. This confirms that the initialization of lastCommittedIndex in BallotBox is properly supported by the BallotBoxOptions class.

• BallotBoxOptions.java implementation:

  public long getLastCommittedIndex() {
      return lastCommittedIndex;
  }
  
  public void setLastCommittedIndex(long lastCommittedIndex) {
      this.lastCommittedIndex = lastCommittedIndex;
  }

This ensures that the lastCommittedIndex is correctly managed and can be used effectively in BallotBox.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify that `getLastCommittedIndex` is implemented and returns expected values across restarts.

# Test: Check the implementation of `getLastCommittedIndex` in BallotBoxOptions.java.
ast-grep --lang java --pattern $'class BallotBoxOptions {
  long getLastCommittedIndex() {
    $$$
  }
}'

Length of output: 106

---

---

Script:

#!/bin/bash
# Search for the definition of `getLastCommittedIndex` method across the codebase.
rg 'long getLastCommittedIndex\('

Length of output: 670

---

---

Script:

#!/bin/bash
# Display the implementation of `getLastCommittedIndex` in BallotBoxOptions.java
rg -A 5 'public long getLastCommittedIndex\(' jraft-core/src/main/java/com/alipay/sofa/jraft/option/BallotBoxOptions.java

Length of output: 326

jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java (1)

2071-2072: Verify the implementation logic for consistency in getLastCommittedIndex after node leadership changes.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5370279 and e7bf34b.

Files selected for processing (2)

• jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java (4 hunks)
• jraft-extension/java-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/HybridLogStorage.java (2 hunks)

Files skipped from review as they are similar to previous changes (2)

• jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java
• jraft-extension/java-log-storage-impl/src/main/java/com/alipay/sofa/jraft/storage/HybridLogStorage.java

[fengjiachun]

LGTM