feat: only retains request header to reduce memory during replication #1191
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Leader
participant Replicator
participant Follower
Leader->>Replicator: Create AppendEntriesRequest/InstallSnapshotRequest
Replicator->>RpcRequestHeader: Extract metadata (prevLogIndex, prevLogTerm, entriesCount, dataBytes, meta)
Replicator->>Follower: Send RPC (AppendEntries/InstallSnapshot)
Follower-->>Replicator: Responds with RPC Response
Replicator->>Replicator: Handle response using RpcRequestHeader
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR reduces memory consumption during replication by introducing a new RpcRequestHeader that retains only essential metadata from requests.
- Introduces RpcRequestHeader to encapsulate minimal request data
- Updates replicator methods and callbacks to utilize RpcRequestHeader instead of holding full request objects
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java | Added a new class to store minimal header data for RPC requests |
| jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java | Modified request handling and callbacks to use RpcRequestHeader for improved memory efficiency |
jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java
Outdated
Show resolved
Hide resolved
jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java (2)
21-26: Update the class documentation to reflect both request types.The class comment mentions only
AppendEntriesRequest, but the implementation also handlesInstallSnapshotRequest(via the snapshot constructor). Update the documentation to accurately reflect this dual purpose./** - * AppendEntriesRequest header, which only keeps the request metadata info without the data. + * RPC request header for AppendEntriesRequest and InstallSnapshotRequest, which only keeps the request metadata info without the data. * It's designed to reduce the memory consumption for RPC. * @author dennis * */
52-70: Consider making getter visibility consistent.The
getMeta()method is public while all other getters are package-private. Since the class itself is package-private, consider making all getters package-private for consistency.- public SnapshotMeta getMeta() { + SnapshotMeta getMeta() { return meta; }jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java (1)
823-825: Extract duplicated RpcRequestHeader creation logic.The logic for creating
RpcRequestHeaderfromAppendEntriesRequestis duplicated. Consider extracting this to a helper method to follow the DRY principle.Add a helper method:
private static RpcRequestHeader createHeaderFromAppendEntriesRequest(final AppendEntriesRequest request) { return new RpcRequestHeader( request.getPrevLogIndex(), request.getPrevLogTerm(), request.getEntriesCount(), request.getData() != null ? request.getData().size() : 0 ); }Then use it in both locations:
-RpcRequestHeader header = new RpcRequestHeader(request.getPrevLogIndex(), - request.getPrevLogTerm(), request.getEntriesCount(), request.getData()!=null?request.getData().size():0); +RpcRequestHeader header = createHeaderFromAppendEntriesRequest(request);Also applies to: 1693-1694
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java(9 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java (1)
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/RaftOutter.java (2)
RaftOutter(22-3114)SnapshotMeta(1818-3070)
🔇 Additional comments (5)
jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java (1)
27-71: LGTM! Well-designed class for memory optimization.The immutable design with final fields is excellent for thread safety and aligns well with the PR's objective to reduce memory consumption. The class effectively encapsulates only the essential metadata needed for RPC response handling.
jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java (4)
428-445: Good refactoring to reduce memory footprint.The change from
MessagetoRpcRequestHeaderin theRpcResponseclass effectively reduces memory consumption by avoiding retention of full request payloads during RPC processing.
696-697: Correct usage of the snapshot constructor.The code properly creates a lightweight
RpcRequestHeadercontaining only the snapshot metadata, which is sufficient for handling the install snapshot response.
1411-1412: Efficient use of pre-calculated data size.Good improvement - using
getDataBytes()from the header avoids redundant calculations and potential null pointer issues.
1261-1379:Details
✅ Verification successful
Verify that all request field accesses are properly migrated.
The refactoring correctly updates all method signatures and RPC callbacks to use
RpcRequestHeader. The changes maintain the same functionality while reducing memory usage.Let me verify that no direct accesses to the original request fields remain in the response handling code:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining direct accesses to protobuf request fields in response handling # Search for potential missed conversions in Replicator.java echo "Checking for potential missed request field accesses in response handling..." rg -A 5 -B 5 "request\.(get|has)" jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java | grep -E "(onRpcReturned|onAppendEntriesReturned|onInstallSnapshotReturned)" -A 10 -B 10 || echo "No concerning patterns found" # Verify all RpcRequestHeader usages are consistent echo -e "\nVerifying RpcRequestHeader usage patterns..." ast-grep --pattern 'new RpcRequestHeader($$$)' jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.javaLength of output: 5906
All request field accesses properly migrated
No direct accesses to protobuf request fields remain in response handling. The only remaining
request.getXxx()calls occur within RPC callbacks to construct the newRpcRequestHeader, which is intentional.
- In
onAppendEntriesReturnedandonInstallSnapshotReturned,requestrefers toRpcRequestHeader, not the original protobuf object.- All direct protobuf field accesses are confined to header construction in the RPC closures.
No further changes needed.
There was a problem hiding this comment.
Pull Request Overview
This PR reduces memory usage in the replication module by replacing full RPC request retention with a lightweight header. It introduces a new RpcRequestHeader type and updates both production code and tests to use it.
- Added
RpcRequestHeaderto capture only essential metadata for AppendEntries and Snapshot RPCs. - Refactored
Replicatorand its innerRpcResponseto carryRpcRequestHeaderinstead of fullMessagerequests. - Updated unit tests to wrap requests with the new header type.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| RpcRequestHeader.java | New header class storing only metadata for RPC requests |
| Replicator.java | Replaced Message request parameters with RpcRequestHeader and updated metrics calls |
| ReplicatorTest.java | Updated test calls to pass RpcRequestHeader instances |
Comments suppressed due to low confidence (2)
jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java:33
- [nitpick] The field name
dataBytessuggests a byte length but it actually counts entries; consider renaming todataCountor similar for clarity.
final int dataBytes;
jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java:1690
- [nitpick] There are two spaces between
finalandRpcRequestHeader; align spacing to project style for consistency.
final RpcRequestHeader header = new RpcRequestHeader(request);
jraft-core/src/main/java/com/alipay/sofa/jraft/core/RpcRequestHeader.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
killme2008
changed the title
Motivation:
Retains only essential information while awaiting a response during replication.
Modification:
Currently, replicator sends entries asynchronously, but the RPC invocation callback retains the entire request in memory, delaying its release. This is especially problematic for large requests, as it leads to high memory usage.
This PR introduces
RpcRequestHeaderforInstallSnapshotRequestandAppendEntriesRequest, which retains only essential information while awaiting a response. This should significantly reduce memory consumption during replication.Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.
Summary by CodeRabbit