Learner source #1163
Learner source #1163
Conversation
WalkthroughThe changes in this pull request primarily involve a significant update to the handling of learners within the JRaft framework. The representation of learners has shifted from list-based structures to map-based structures, specifically using Changes
Poem
Warning Rate limit exceeded@dbl-x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (60)
jraft-core/src/main/resources/raft.proto (2)
23-24: LGTM! Consider adding documentation for field relationships.The new map fields appropriately extend the message to support learner-source mapping while maintaining backward compatibility.
Consider adding comments to explain:
- The relationship between
learners/old_learnerslists and the new map fields- When to use lists vs. maps in different scenarios
34-35: Consider using sequential field numbers in SnapshotMeta.While the field numbers (9, 10) match EntryMeta for similar fields, they skip numbers 7-8 in SnapshotMeta. Consider using sequential numbers (7, 8) instead to maintain cleaner field ordering.
- map<string, string> learner_with_source = 9; - map<string, string> old_learner_with_source = 10; + map<string, string> learner_with_source = 7; + map<string, string> old_learner_with_source = 8;jraft-example/src/main/java/com/alipay/sofa/jraft/example/counter/TestCliList.java (3)
29-32: Add class-level documentationThe class lacks proper documentation explaining its purpose and usage. Consider adding comprehensive JavaDoc to describe the class's role as a CLI example.
/** - * + * Example demonstrating how to use CliService to list peers in a JRaft cluster. + * This class provides a simple command-line interface to retrieve and display + * the list of peers in a specified JRaft group. */
37-38: Translate comments to English for consistencyThe codebase should maintain consistent use of English in comments.
- // 创建并初始化 CliService + // Create and initialize CliService
34-34: Consider making GROUP configurableThe group name is hardcoded as a constant. Consider making it configurable through command-line arguments or system properties for better flexibility.
- private static final String GROUP = "jraft-example-group"; + private static final String DEFAULT_GROUP = "jraft-example-group"; + private static String getGroupName() { + return System.getProperty("jraft.group", DEFAULT_GROUP); + }jraft-example/src/main/java/com/alipay/sofa/jraft/example/counter/TestCli.java (2)
27-30: Add comprehensive class documentationAs an example code, this class should have proper JavaDoc documentation explaining:
- The purpose of this example
- Usage instructions
- Expected behavior
- Required setup/configuration
/** + * Example demonstrating how to use JRaft's CliService to manage cluster peers. + * + * This example shows how to: + * 1. Initialize a CliService + * 2. Remove a peer from a JRaft group + * + * Usage: + * Run this example after setting up a JRaft cluster with the specified group name + * and peer addresses. */
30-46: Consider adding safety measures for cluster managementAs this example demonstrates cluster peer management, which is a critical operation, consider enhancing it with:
- Confirmation prompt before removing a peer
- Validation of cluster health/status before modification
- Verification of successful peer removal
- Logging of operation details for audit purposes
- Documentation of recovery procedures
jraft-core/src/main/resources/cli.proto (2)
82-82: Consider adding field documentation for learner_with_sourceThe new map field is well-structured and maintains backward compatibility. Consider adding a comment to document the mapping relationship between learners and their sources, which would help other developers understand the purpose of this field.
+ // Maps learner peer IDs to their source peer IDs map<string, string> learner_with_source = 3;
107-108: Consider adding field documentation for learner source mapsThe new map fields follow a consistent naming pattern and maintain backward compatibility. Consider adding comments to document these fields' purposes and their relationship with the existing learners fields.
+ // Maps learner peer IDs to their source peer IDs before the operation map<string, string> old_learner_with_source = 3; + // Maps learner peer IDs to their source peer IDs after the operation map<string, string> new_learner_with_source = 4;jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/AddLearnersRequestProcessorTest.java (1)
Line range hint
39-63: Test needs to be updated for map-based learner sourcesThe test should be refactored to verify the new map-based learner source implementation. Consider these changes:
- Update
createRequest()to include learner source information- Modify verification to check correct learner-to-source mapping
- Add test cases for various learner source scenarios
Here's a suggested approach:
@Override public void verify(final String interest, final Node node, final ArgumentCaptor<Closure> doneArg) { assertEquals(interest, AddLearnersRequest.class.getName()); + Map<PeerId, PeerId> expectedLearners = new HashMap<>(); + expectedLearners.put(new PeerId("learner", 8082), null); // or specific source + expectedLearners.put(new PeerId("test", 8182), null); + expectedLearners.put(new PeerId("test", 8183), null); Mockito.verify(node).addLearners( - Arrays.asList(new PeerId("learner", 8082), new PeerId("test", 8182), new PeerId("test", 8183)), + expectedLearners, doneArg.capture());jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessorTest.java (1)
56-60: LGTM! Consider minor improvements for better test maintainability.The implementation correctly verifies the new map-based learner representation. Consider these improvements:
- Add a comment explaining the use of
Configuration.NULL_PEERID- Extract the learner PeerIds to constants for better reusability and maintenance
+ private static final PeerId LEARNER_1 = new PeerId("learner", 8082); + private static final PeerId LEARNER_2 = new PeerId("test", 8182); + private static final PeerId LEARNER_3 = new PeerId("test", 8183); @Override public void verify(final String interest, final Node node, final ArgumentCaptor<Closure> doneArg) { assertEquals(interest, ResetLearnersRequest.class.getName()); + // Using NULL_PEERID as the map value since we only need to track the learner PeerIds Map<PeerId, PeerId> learners = new ConcurrentHashMap<>(); - learners.put(new PeerId("learner", 8082), Configuration.NULL_PEERID); - learners.put(new PeerId("test", 8182), Configuration.NULL_PEERID); - learners.put(new PeerId("test", 8183), Configuration.NULL_PEERID); + learners.put(LEARNER_1, Configuration.NULL_PEERID); + learners.put(LEARNER_2, Configuration.NULL_PEERID); + learners.put(LEARNER_3, Configuration.NULL_PEERID);jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/GetPeersRequestProcessor.java (2)
72-76: LGTM: Consider adding documentationThe learner source mapping logic is well-implemented with proper null checks. Consider adding a brief comment explaining the purpose of this mapping for future maintainers.
+ // Populate the learner-to-source mapping in the response if (learnerWithSource != null && !learnerWithSource.isEmpty()) {
Line range hint
56-76: Consider updating class documentationThis implementation successfully bridges the transition from list-based to map-based learner management. Consider updating the class-level documentation to reflect this enhanced capability of tracking learner sources, which would help maintain architectural clarity.
jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/RemoveLearnersRequestProcessor.java (2)
57-58: Consider adding documentation for the Map structure.The change to Map-based representation is good, but it would be helpful to document what the key-value relationship represents in the Map<PeerId, PeerId>.
78-82: Consider using Map.containsKey for better performance.The logic is correct, but since removingLearners is a List, the contains check could be O(n). Consider converting removingLearners to a Set for O(1) lookups if the list size could be large.
- final List<PeerId> removingLearners = new ArrayList<>(request.getLearnersCount()); + final Set<PeerId> removingLearners = new HashSet<>(request.getLearnersCount());jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/AbstractCliRequestProcessorTest.java (1)
62-64: Consider enhancing test coverage for learner source mappingWhile the implementation correctly uses ConcurrentHashMap for thread-safety, consider adding test cases that verify scenarios where learners have actual source peers instead of just NULL_PEERID. This would better validate the new map-based structure's capability to track learner sources.
Example enhancement:
Map<PeerId, PeerId> learners = new ConcurrentHashMap<>(); for (int i = 0; i < n; i++) { - learners.put(JRaftUtils.getPeerId("learner:" + (8081 + i)), Configuration.NULL_PEERID); + // Add mix of learners with and without sources + PeerId learner = JRaftUtils.getPeerId("learner:" + (8081 + i)); + PeerId source = (i % 2 == 0) ? Configuration.NULL_PEERID : + JRaftUtils.getPeerId("source:" + (8081 + i)); + learners.put(learner, source); }jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationEntryTest.java (1)
72-74: LGTM! Consider adding more test cases for learner-source relationships.The changes correctly adapt to the new learner management architecture using Map<PeerId, PeerId>. The test cases properly validate that:
- Adding a non-existing peer (8084) as a learner makes the configuration invalid
- Adding an existing peer (8081) as a learner makes the configuration invalid
Consider adding additional test cases to verify:
- Adding learners with different source peers
- Validating learner-source relationships
- Edge cases with NULL_PEERID vs actual source peers
jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessor.java (2)
57-64: Verify error handling completenessWhile the peer parsing logic remains sound, the error response could be more informative by including:
- The current learners state
- Validation results for all peers, not just the first invalid one
Consider enhancing the error response like this:
- return RpcFactoryHelper.responseFactory().newResponse(defaultResp(), RaftError.EINVAL, - "Fail to parse peer id %s", peerStr); + return RpcFactoryHelper.responseFactory().newResponse(defaultResp(), RaftError.EINVAL, + "Failed to parse peer id %s. Current learners: %s", peerStr, oldLearners);
57-88: Consider adding migration support for backward compatibilitySince this represents a significant change in how learners are managed (list-based to map-based), consider:
- Adding version checking or migration support for clusters with mixed versions
- Documenting the new learner source mapping approach
- Adding validation to ensure all learners have valid sources
Would you like assistance in designing a backward compatibility layer or documentation structure?
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/V2Encoder.java (1)
95-102: LGTM with a minor optimization suggestionThe implementation correctly handles the new map-based learner structure. Consider extracting the repeated pattern:
- if (learners != null && hasPeers(learners.keySet())) { - encodeLearners(builder, learners); - } - final Map<PeerId, PeerId> oldLearners = log.getOldLearners(); - if (oldLearners != null && hasPeers(oldLearners.keySet())) { - encodeOldLearners(builder, oldLearners); - } + private void encodeLearnerMapIfPresent(PBLogEntry.Builder builder, + Map<PeerId, PeerId> learnerMap, + BiConsumer<PBLogEntry.Builder, Map<PeerId, PeerId>> encoder) { + if (learnerMap != null && hasPeers(learnerMap.keySet())) { + encoder.accept(builder, learnerMap); + } + }Then use it as:
encodeLearnerMapIfPresent(builder, learners, this::encodeLearners); encodeLearnerMapIfPresent(builder, oldLearners, this::encodeOldLearners);jraft-core/src/test/java/com/alipay/sofa/jraft/entity/codec/v2/LogEntryV2CodecFactoryTest.java (1)
104-111: Consider making createLearners more flexibleWhile the implementation is correct, consider enhancing the helper method to allow specification of both key and value PeerIds, making it more versatile for different test scenarios.
-private Map<PeerId, PeerId> createLearners(final String... peers) { +private Map<PeerId, PeerId> createLearners(final String... peers) { Map<PeerId, PeerId> ret = new ConcurrentHashMap<>(); for (String s : peers) { PeerId e = new PeerId(); e.parse(s); - ret.put(e, Configuration.NULL_PEERID); + ret.put(e, Configuration.NULL_PEERID); } return ret; } + +// Alternative method for cases requiring specific source peers +private Map<PeerId, PeerId> createLearnersWithSources(final String[] learners, final String[] sources) { + assert learners.length == sources.length : "Learners and sources must have same length"; + Map<PeerId, PeerId> ret = new ConcurrentHashMap<>(); + for (int i = 0; i < learners.length; i++) { + PeerId learner = new PeerId(); + learner.parse(learners[i]); + PeerId source = new PeerId(); + source.parse(sources[i]); + ret.put(learner, source); + } + return ret; +}jraft-core/src/test/java/com/alipay/sofa/jraft/entity/PeerIdTest.java (1)
152-164: Consider enhancing the replication group test coverageWhile the new test method covers basic scenarios, consider these improvements:
- Replace
assert peerId != nullwithAssert.assertNotNull(peerId)for consistency with JUnit style- Add test cases for:
- Empty replication group
- Special characters in replication group
- Maximum length validation (if applicable)
Here's a suggested enhancement:
@Test public void testParseWithReplicationGroup() { String toStringContent = "192.168.1.1:8888:::replicationGroup0"; PeerId peerId = PeerId.parsePeer(toStringContent); - assert peerId != null; + Assert.assertNotNull(peerId); Assert.assertEquals(toStringContent, peerId.toString()); String toStringContent1 = "192.168.1.1:8888"; PeerId peerId1 = PeerId.parsePeer(toStringContent1); - assert peerId1 != null; + Assert.assertNotNull(peerId1); Assert.assertEquals(toStringContent1, peerId1.toString()); + + // Test empty replication group + String toStringContent2 = "192.168.1.1:8888:::"; + PeerId peerId2 = PeerId.parsePeer(toStringContent2); + Assert.assertNotNull(peerId2); + Assert.assertEquals(toStringContent2, peerId2.toString()); }jraft-core/src/main/java/com/alipay/sofa/jraft/entity/LogEntry.java (2)
50-52: Consider enhancing field documentationWhile the change to Map is appropriate, the documentation could better explain the key-value relationship (learner to source mapping).
Consider updating the comments to:
- /** log entry current learners */ + /** Map of learner peers to their corresponding source peers */ - /** log entry old learners */ + /** Map of old learner peers to their corresponding source peers */
60-74: Consider implementing defensive copyingThe getter methods return direct references to the internal maps, which could lead to unintended modifications. Consider implementing defensive copying.
public Map<PeerId, PeerId> getLearners() { - return this.learners; + return this.learners != null ? new HashMap<>(this.learners) : null; } public void setLearners(final Map<PeerId, PeerId> learners) { - this.learners = learners; + this.learners = learners != null ? new HashMap<>(learners) : null; } public Map<PeerId, PeerId> getOldLearners() { - return this.oldLearners; + return this.oldLearners != null ? new HashMap<>(this.oldLearners) : null; } public void setOldLearners(final Map<PeerId, PeerId> oldLearners) { - this.oldLearners = oldLearners; + this.oldLearners = oldLearners != null ? new HashMap<>(oldLearners) : null; }jraft-core/src/main/java/com/alipay/sofa/jraft/RouteTable.java (1)
323-337: LGTM: Well-structured learner configuration handling with backward compatibilityThe implementation effectively handles both the new map-based learner-source pairs and maintains backward compatibility with the list-based approach. The code is robust with proper error handling through PeerId parsing.
Consider adding debug logging when processing learners to improve observability:
if (resp.getLearnerWithSourceCount() > 0) { + LOG.debug("Processing {} learner-source pairs for group {}", resp.getLearnerWithSourceCount(), groupId); for (Map.Entry<String, String> entry : resp.getLearnerWithSourceMap().entrySet()) { final PeerId newLearner = new PeerId(); newLearner.parse(entry.getKey()); final PeerId source = new PeerId(); source.parse(entry.getValue()); newConf.addLearner(newLearner, source); } } else { + LOG.debug("Processing legacy learner list for group {}", groupId); for (final String learnerIdStr : resp.getLearnersList()) { final PeerId newLearner = new PeerId(); newLearner.parse(learnerIdStr); newConf.addLearner(newLearner, Configuration.NULL_PEERID); } }jraft-core/src/main/java/com/alipay/sofa/jraft/entity/EnumOutter.java (1)
Line range hint
1-390: Consider protecting manual modifications to generated codeSince this file is generated by the protocol buffer compiler, consider one of these approaches to protect manual modifications:
- Document the manual changes in a separate file that's referenced in the build process
- Create a wrapper class for custom modifications
- Use protobuf plugins to generate the desired documentation automatically
This will help maintain the changes across regenerations of the protobuf code.
jraft-core/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java (2)
93-93: LGTM: Enhanced learner management with source trackingThe change from Set to Map enables tracking both learners and their sources, which is a beneficial architectural improvement for better learner management.
103-108: Consider adding defensive copyingThe getter returns the internal map directly, which could lead to external modifications affecting the internal state. Consider returning an unmodifiable copy of the map.
public Map<PeerId, PeerId> getLearners() { - return this.learners; + return Collections.unmodifiableMap(new HashMap<>(this.learners)); }jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java (1)
94-94: Consider making the learners field finalSince this field is initialized in the constructor and represents a core data structure, consider making it final to prevent accidental reassignment.
- private Map<PeerId, PeerId> learners; + private final Map<PeerId, PeerId> learners;jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java (1)
83-87: Consider documenting the NULL_PEERID usageWhile the transition to ConcurrentHashMap is good, consider adding a comment explaining why NULL_PEERID is used as the value in test scenarios. This would help other developers understand the test setup better.
jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java (1)
162-166: LGTM! Assertions adapted for map-based learner storage.The assertions correctly verify learner sets using containsAll(). Consider using assertEquals() instead of bidirectional containsAll() checks for more precise equality testing.
-assertTrue(oldLearners.containsAll(this.cliService.getLearners(this.groupId, this.conf))); -assertTrue(this.cliService.getLearners(this.groupId, this.conf).containsAll(oldLearners)); +assertEquals(new HashSet<>(oldLearners), new HashSet<>(this.cliService.getLearners(this.groupId, this.conf)));pom.xml (1)
Line range hint
1-494: Consider updating other dependenciesWhile reviewing the POM file, I noticed several other dependencies that could benefit from updates to address security vulnerabilities:
- log4j-core/api/slf4j-impl/jcl (2.17.1): Has known CVEs
- junit (4.13.1): Has known vulnerabilities
- commons-compress (1.21): Has security fixes in newer versions
jraft-core/src/main/java/com/alipay/sofa/jraft/core/FSMCallerImpl.java (1)
626-627: LGTM! Consider adding null checks for defensive programming.The implementation correctly handles the learner-source mapping in the snapshot metadata. However, consider adding null checks for the map entries to make the code more robust.
+if (entry.getKey() != null && entry.getValue() != null) { metaBuilder.putLearnerWithSource(entry.getKey().toString(), entry.getValue().toString()); +}jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/StoreEngine.java (1)
708-708: LGTM! Consider adding documentation.The change correctly implements the new map-based learner validation logic. Consider adding a comment to document that this check validates both regular peers and learners in the configuration.
return allConf.contains(currPeer) || allConf.getLearners().containsKey(currPeer); + // Check if the peer is either a regular member or a learner in the configurationjraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/AddLearnersRequestProcessor.java (1)
58-58: Clarify the usage ofMap<PeerId, PeerId>foroldLearners.Using a
Map<PeerId, PeerId>to represent learners and their sources enhances clarity, but it would be helpful to document the relationship between the key and value. Consider adding comments to explain the mapping from learner IDs to their source IDs for better maintainability.jraft-core/src/main/java/com/alipay/sofa/jraft/entity/PeerId.java (2)
132-138: Ensure consistent use offinalin constructor parametersFor consistency with other constructors, consider declaring the
replicationGroupparameter asfinal.Apply this diff to enhance consistency:
-public PeerId(final Endpoint endpoint, final int idx, final int priority, String replicationGroup) { +public PeerId(final Endpoint endpoint, final int idx, final int priority, final String replicationGroup) {
188-206: Consider refining thetoString()method to avoid extra colonsThe current
toString()implementation may include unnecessary colons when optional fields likeidx,priority, orreplicationGrouphave default values. This could lead to string representations likeip:port:::replicationGroup.Consider adjusting the logic to append colons only when the preceding value is present. For example:
if (this.idx != 0) { buf.append(':').append(this.idx); } if (this.priority != ElectionPriority.Disabled) { + if (this.idx == 0) { buf.append(':'); } buf.append(':').append(this.priority); } if (!StringUtils.isEmpty(this.replicationGroup)) { + if (this.idx == 0 && this.priority == ElectionPriority.Disabled) { buf.append("::"); } + else if (this.idx == 0 || this.priority == ElectionPriority.Disabled) { buf.append(':'); } buf.append(':').append(this.replicationGroup); }This refinement ensures the string representation is clean and unambiguous.
jraft-core/src/main/java/com/alipay/sofa/jraft/conf/Configuration.java (4)
46-46: Consider the access modifier forNULL_PEERIDIntroducing
NULL_PEERIDas apublicconstant may expose it unnecessarily. If it's only used within this class or package, consider changing the access modifier toprivateor package-private.
48-49: Correct grammatical errors in commentsThe comments for
LEARNER_POSTFIXandLEARNER_PREFIXcontain grammatical errors.
- Line 48: Change "learner copy data from leader" to "Learner copies data from the leader."
- Line 49: Change "learner copy data from node that in same replication group" to "Learner copies data from a node in the same replication group."
92-92: Use interface types in method parametersIn the
setLearnersmethod, consider changing the parameter type fromConcurrentHashMap<PeerId, PeerId>toMap<PeerId, PeerId>to allow more flexibility in the types of maps that can be passed.Apply this diff to change the method signature:
- public void setLearners(final ConcurrentHashMap<PeerId, PeerId> learners) { + public void setLearners(final Map<PeerId, PeerId> learners) { this.learners = learners; }
277-280: Optimize string concatenation intoStringmethodIn the
toStringmethod, consider optimizing the string concatenation when appending learner entries. This can improve performance and readability.jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java (2)
307-335: Consolidate duplicate code when processing learners and their sourcesThe code in lines 307-335 handling
oldLearnerWithSourceMapandgetOldLearnersList, as well asnewLearnerWithSourceMapandgetNewLearnersList, contains duplicated logic for parsingPeerIdobjects and adding them to configurations. Consider extracting this repetitive code into a helper method to improve maintainability and reduce duplication.
646-650: Simplify exception handling by removing redundant catch blockThe
catchblock forJRaftExceptionimmediately rethrows the same exception without additional processing:} catch (final JRaftException e) { throw e; }This makes the catch block unnecessary. Removing it will simplify the code and allow the exception to propagate naturally.
Apply this diff to simplify the exception handling:
- } catch (final JRaftException e) { - throw e; - } catch (final Exception e) { + } catch (final Exception e) {jraft-core/src/main/java/com/alipay/sofa/jraft/storage/impl/LogManagerImpl.java (2)
668-681: Consider refactoring duplicated code for parsing learners with sourcesThe code block parsing learners and their sources from
meta.getOldLearnerWithSourceMap()is similar to the one inconfFromMeta(). Extracting this logic into a shared private method can improve maintainability and reduce code duplication.
693-706: Consider refactoring duplicated code for parsing learners with sourcesThe logic for parsing learners and their sources from
meta.getLearnerWithSourceMap()duplicates the code inoldConfFromMeta(). Refactoring this into a shared helper method would enhance readability and maintainability.jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java (1)
879-889: Refactor duplicated code for handling learnersThe loops handling
entry.getLearners()andentry.getOldLearners()are similar. Consider refactoring them into a helper method to reduce code duplication and enhance maintainability.You could extract a helper method like this:
private void addLearnersWithSource(BiConsumer<String, String> consumer, Map<PeerId, PeerId> learners) { for (final Map.Entry<PeerId, PeerId> learnerEntry : learners.entrySet()) { PeerId learner = learnerEntry.getKey(); PeerId source = learnerEntry.getValue(); consumer.accept(learner.toString(), source.toString()); } }Then update the existing code:
if (entry.getLearners() != null) { - for (final Map.Entry<PeerId, PeerId> learnerEntry : entry.getLearners().entrySet()) { - PeerId learner = learnerEntry.getKey(); - PeerId source = learnerEntry.getValue(); - emb.putLearnerWithSource(learner.toString(), source.toString()); - } + addLearnersWithSource(emb::putLearnerWithSource, entry.getLearners()); } if (entry.getOldLearners() != null) { - for (final Map.Entry<PeerId, PeerId> learnerEntry : entry.getOldLearners().entrySet()) { - PeerId learner = learnerEntry.getKey(); - PeerId source = learnerEntry.getValue(); - emb.putOldLearnerWithSource(learner.toString(), source.toString()); - } + addLearnersWithSource(emb::putOldLearnerWithSource, entry.getOldLearners()); }jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/LogOutter.java (3)
Line range hint
1-14: Avoid modifying generated code; make changes in the.protofile insteadThis file is generated by the Protocol Buffers compiler (
protoc), as indicated by the header comment. Direct modifications to generated code will be overwritten when the code is regenerated. To apply changes, please modify thelog.protofile and then regenerate the Java classes usingprotoc.
2110-2131: Consider throwingNoSuchElementExceptionwhen the key is missingIn the
getLearnerWithSourceOrThrowmethod, when a key is not present in the map, it's more appropriate to throw aNoSuchElementExceptionto indicate that the requested element does not exist.Apply this diff to change the exception:
if (!map.containsKey(key)) { - throw new java.lang.IllegalArgumentException(); + throw new java.util.NoSuchElementException("Key not found: " + key); }
2242-2263: Consider throwingNoSuchElementExceptionwhen the key is missingIn the
getOldLearnerWithSourceOrThrowmethod, replacing theIllegalArgumentExceptionwith aNoSuchElementExceptionwould more accurately reflect the absence of the key in the map.Apply this diff to change the exception:
if (!map.containsKey(key)) { - throw new java.lang.IllegalArgumentException(); + throw new java.util.NoSuchElementException("Key not found: " + key); }jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java (1)
30-30: Consider usingHashMapinstead ofConcurrentHashMapif thread safety is not requiredThe
learnersmap is declared as aConcurrentHashMap, but if it's not accessed concurrently from multiple threads, a regularHashMapmay offer better performance due to lower overhead.Apply this diff to switch to
HashMap:-import java.util.concurrent.ConcurrentHashMap; +import java.util.HashMap; ... -Map<PeerId, PeerId> learners = new ConcurrentHashMap<>(); +Map<PeerId, PeerId> learners = new HashMap<>();Also applies to: 647-647
jraft-core/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java (6)
666-667: Consider using aSet<PeerId>instead ofMap<PeerId, PeerId>for learners.Since the values in the
learnersmap are allConfiguration.NULL_PEERIDand are not utilized, it would be more appropriate to use aSet<PeerId>to store learners. This simplifies the data structure and enhances readability.Apply this diff to modify the data structure:
- Map<PeerId, PeerId> learners = new ConcurrentHashMap<>(); - learners.put(learnerPeer, Configuration.NULL_PEERID); + Set<PeerId> learners = new HashSet<>(); + learners.add(learnerPeer);
682-682: UpdateConfigurationconstructor to acceptSet<PeerId>.After changing
learnersto aSet<PeerId>, ensure that theConfigurationconstructor reflects this change. If the constructor accepts aSet<PeerId>for learners, pass the set directly.Apply this diff:
- nodeOptions.setInitialConf(new Configuration(Collections.singletonList(peer), learners)); + nodeOptions.setInitialConf(new Configuration(Collections.singletonList(peer), learners));If the
Configurationclass does not have a constructor that accepts aSet<PeerId>, consider adding one or converting the set to the required type.
732-735: Use aSet<PeerId>for learners and adjust initialization accordingly.Modify the learners' data structure to a
Set<PeerId>and update the population logic to match.Apply this diff:
- final Map<PeerId, PeerId> learners = new ConcurrentHashMap<>(); + final Set<PeerId> learners = new HashSet<>(); for (int i = 0; i < 3; i++) { - learners.put(new PeerId(TestUtils.getMyIp(), TestUtils.INIT_PORT + 3 + i), Configuration.NULL_PEERID); + learners.add(new PeerId(TestUtils.getMyIp(), TestUtils.INIT_PORT + 3 + i)); }
743-743: Simplify iteration over learners by iterating directly over the set.With
learnersas aSet<PeerId>, you can iterate directly without callingkeySet().Apply this diff:
- for (final PeerId peer : learners.keySet()) { + for (final PeerId peer : learners) {
811-813: Update learners data structure toSet<PeerId>in this context.Consistently use a
Set<PeerId>for the learners and adjust the code accordingly.Apply this diff:
- Map<PeerId, PeerId> learners = new ConcurrentHashMap<>(); + Set<PeerId> learners = new HashSet<>(); PeerId learnerPeer = new PeerId(TestUtils.getMyIp(), TestUtils.INIT_PORT + 3); - learners.put(learnerPeer, Configuration.NULL_PEERID); + learners.add(learnerPeer); cluster.setLearners(learners);
871-871: Ensure consistency in learners' data structure when adding learners.After changing
learnersto aSet<PeerId>, verify that methods likeaddLearnersare updated to accept the new data structure or that the appropriate conversions are made.If
addLearnersexpects a list, you might need to convert the set:- leader.addLearners(Collections.singletonList(learnerPeer), done); + leader.addLearners(new ArrayList<>(learners), done);Ensure that the
addLearnersmethod accepts the correct type corresponding to yourlearnersdata structure.jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java (2)
3220-3252: OptimizegetTargetSourcePeerLogic for Clarity and PerformanceConsider refactoring
getTargetSourcePeerto improve readability and performance using modern Java features.Here is a possible refactored version:
public PeerId getTargetSourcePeer(String replicationGroup) { if (StringUtils.isEmpty(replicationGroup)) { // Use leader as default source peer return getLeaderId(); } else { Configuration configuration = this.conf.getConf(); List<PeerId> peers = configuration.getPeers(); if (peers.isEmpty()) { return getLeaderId(); } Map<PeerId, PeerId> learnerWithSource = configuration.getLearners(); - PeerId targetPeerId = null; - int minPeerLoad = Integer.MAX_VALUE; - for (PeerId peerId : peers) { - if (!replicationGroup.equals(peerId.getReplicationGroup())) { - continue; - } - int learnerCount = 0; - for (PeerId source : learnerWithSource.values()) { - if (source.equals(peerId)) { - learnerCount++; - } - } - if (learnerCount < minPeerLoad) { - targetPeerId = peerId; - minPeerLoad = learnerCount; - } - } + PeerId targetPeerId = peers.stream() + .filter(peerId -> replicationGroup.equals(peerId.getReplicationGroup())) + .min(Comparator.comparingInt(peerId -> + (int) learnerWithSource.values().stream().filter(peerId::equals).count())) + .orElse(getLeaderId()); return targetPeerId; } }
3254-3259: RefactorcheckPeersMethods to Eliminate DuplicationYou have two overloaded
checkPeersmethods with similar validation logic. Refactoring them into a single generic method or abstracting the common logic can enhance maintainability.jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/V2Decoder.java (2)
94-111: Consider refactoring duplicate code in learners handlingThe code blocks handling
learnersandoldLearners(lines 94-111 and lines 113-130) are nearly identical. To improve maintainability and reduce code duplication, consider extracting the shared logic into a helper method.
113-130: Consider refactoring duplicate code in old learners handlingSimilar to the
learnersblock, theoldLearnershandling code has duplicate logic. Extracting this into a common helper method will enhance readability and simplify future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (42)
jraft-core/src/main/java/com/alipay/sofa/jraft/Node.java(3 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/RouteTable.java(2 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/conf/Configuration.java(10 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/conf/ConfigurationEntry.java(1 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java(5 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/core/FSMCallerImpl.java(2 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java(18 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java(2 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/EnumOutter.java(4 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/LocalFileMetaOutter.java(35 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/LogEntry.java(3 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/PeerId.java(8 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/LogOutter.java(60 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/V2Decoder.java(2 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/V2Encoder.java(3 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/AddLearnersRequestProcessor.java(3 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/GetPeersRequestProcessor.java(3 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/RemoveLearnersRequestProcessor.java(3 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessor.java(2 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/storage/impl/LogManagerImpl.java(2 hunks)jraft-core/src/main/resources/cli.proto(2 hunks)jraft-core/src/main/resources/log.proto(1 hunks)jraft-core/src/main/resources/raft.proto(2 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationEntryTest.java(1 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationTest.java(4 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java(4 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java(11 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java(6 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/entity/PeerIdTest.java(7 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/entity/codec/v2/LogEntryV2CodecFactoryTest.java(4 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/AbstractCliRequestProcessorTest.java(2 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/AddLearnersRequestProcessorTest.java(1 hunks)jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessorTest.java(2 hunks)jraft-example/src/main/java/com/alipay/sofa/jraft/example/counter/CounterServer.java(3 hunks)jraft-example/src/main/java/com/alipay/sofa/jraft/example/counter/TestCli.java(1 hunks)jraft-example/src/main/java/com/alipay/sofa/jraft/example/counter/TestCliList.java(1 hunks)jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java(2 hunks)jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java(8 hunks)jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java(6 hunks)jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/StoreEngine.java(1 hunks)jraft-rheakv/rheakv-pd/src/main/java/com/alipay/sofa/jraft/rhea/PlacementDriverServer.java(1 hunks)pom.xml(1 hunks)
🔇 Additional comments (76)
jraft-core/src/main/resources/log.proto (2)
20-21: LGTM! Schema evolution maintains backward compatibility.
The new map fields are correctly added with higher field numbers and are optional by default in proto2, ensuring backward compatibility with existing clients.
Line range hint 8-9: Document the relationship between list and map fields.
The message now contains both list-based (learners, old_learners) and map-based (learner_with_source, old_learner_with_source) representations of learners. Please clarify:
- The relationship between these fields
- Whether both need to be maintained
- Migration strategy for existing data
Consider adding comments to explain the purpose and relationship of these fields.
repeated bytes learners = 8;
repeated bytes old_learners = 9;
+ // Maps learner PeerId to its source PeerId, replacing the legacy learners field
map<string, string> learner_with_source = 10;
+ // Maps old learner PeerId to its source PeerId, replacing the legacy old_learners field
map<string, string> old_learner_with_source = 11;Also applies to: 20-21
jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessorTest.java (1)
24-25: LGTM! Import changes align with the new map-based learner representation.
jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/GetPeersRequestProcessor.java (2)
19-21: LGTM: Import changes are appropriate
The addition of the Map import is necessary for the new learner source mapping functionality.
56-56: LGTM: Clear and appropriate variable declaration
The new map variable for learner source mapping aligns with the architectural changes and has a descriptive name.
jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/RemoveLearnersRequestProcessor.java (2)
21-21: LGTM!
The Map import is correctly added to support the new data structure for learner representation.
67-67: LGTM!
The variable usage is consistent and the logging provides good operational visibility.
Also applies to: 71-71
jraft-core/src/test/java/com/alipay/sofa/jraft/rpc/impl/cli/AbstractCliRequestProcessorTest.java (1)
20-21: LGTM! Import changes align with new map-based implementation
The addition of Map and ConcurrentHashMap imports properly supports the transition to map-based learner management.
jraft-core/src/main/java/com/alipay/sofa/jraft/conf/ConfigurationEntry.java (2)
112-113: LGTM! Thread-safe implementation maintained.
The adaptation to use keySet() from the new map-based structure is implemented correctly. The creation of a new HashSet maintains thread safety when combining learners from both configurations.
118-118: LGTM! Verify if map values are utilized elsewhere.
The change to containsKey() is correct for the map-based structure. Since we're only checking for key existence here, please verify if the mapped values (source information) are being utilized appropriately in other parts of the codebase.
jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessor.java (2)
21-21: LGTM: Required import for new Map usage
The addition of the Map import is consistent with the transition from list-based to map-based learner handling.
69-88:
Critical: Implementation is incomplete
There are several issues that need to be addressed:
- The core reset functionality is currently commented out
- There's a TODO comment in Chinese indicating additional work is needed
- The method returns null without completing the operation
Please:
- Translate the TODO comment to English for better international collaboration
- Implement the learner source computation logic
- Re-enable the reset implementation with the new map-based structure
- Add appropriate error handling if the source computation fails
The TODO comment translates to: "After receiving the request here, we should calculate the Source corresponding to the Learner, and then call Node to perform the reset operation"
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/V2Encoder.java (3)
22-22: LGTM: Import addition is appropriate
The Map import is correctly placed and necessary for the new learner source mapping functionality.
63-66: LGTM: Learner encoding implementation looks correct
The implementation properly handles the mapping of learners to their sources. The method correctly uses the protobuf builder's putLearnerWithSource method for encoding.
Please ensure that the protobuf schema (log.proto) has been updated to include the learner_with_source field in the PBLogEntry message.
69-72: LGTM: Old learner encoding maintains consistency
The implementation mirrors the encodeLearners method, maintaining a consistent approach for both current and old learners.
jraft-core/src/test/java/com/alipay/sofa/jraft/entity/codec/v2/LogEntryV2CodecFactoryTest.java (3)
22-23: LGTM: Required imports for new Map-based implementation
The new imports are correctly placed and necessary for the Map-based learner implementation.
Line range hint 56-83: LGTM: Comprehensive test coverage for Map-based learner implementation
The test method has been properly updated to:
- Use Map<PeerId, PeerId> instead of List
- Validate both normal and old learners
- Maintain comprehensive assertions
Line range hint 87-102: LGTM: Well-structured assertions for Map-based implementation
The assertion method properly validates both the unchanged peer list and the new Map-based learner structure.
jraft-core/src/test/java/com/alipay/sofa/jraft/entity/PeerIdTest.java (3)
20-20: LGTM!
The added import is necessary for the new test assertions.
72-75: String format changes look consistent
The updated test assertions correctly reflect the new PeerId string format that includes replication group information. The format is well-documented in the comment: ip:port::priority:replicationGroup.
Please ensure this format change is documented in the main PeerId class as well.
Also applies to: 91-91, 108-108
124-125: LGTM!
The test case has been appropriately updated to validate parsing failure with an invalid number of segments, considering the new format that includes replication group.
jraft-example/src/main/java/com/alipay/sofa/jraft/example/counter/CounterServer.java (2)
19-19: LGTM: Import aligns with CLI service usage
The addition of CliService import corresponds to enabling CLI service in the node options.
Line range hint 107-133: Verify timeouts for production environment
The election timeout (1s) and snapshot interval (30s) are configured for testing. These values might need adjustment in production:
- Short election timeouts could cause unnecessary leader elections in case of temporary network issues
- Snapshot interval should be tuned based on your data change rate and recovery time objectives
jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationTest.java (6)
84-84: Verify the new configuration string format with trailing colons
The configuration string format has been updated to include trailing colons for each peer entry. Ensure this change is documented in the Configuration class and is consistent with the new map-based implementation.
107-107: LGTM - Format change consistent with previous test
140-142: Verify learner source handling with NULL_PEERID
The test now explicitly sets NULL_PEERID as the source for learners. Ensure this is the expected default behavior when no specific source is provided.
145-146: LGTM - Updated to map-based learner verification
The change from contains to containsKey correctly reflects the new map-based learner storage implementation.
148-148: Verify the new learner string representation format
The configuration string now includes explicit source mapping for learners using the format learner/<learner_id>-><source_id>. Ensure this format is documented in the Configuration class.
159-159: LGTM - Invalid configuration test case
Good test case verifying that a peer cannot simultaneously be both a regular peer and a learner.
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/LogEntry.java (1)
21-21: LGTM!
The Map import is correctly added to support the new data structure for learners.
jraft-rheakv/rheakv-pd/src/main/java/com/alipay/sofa/jraft/rhea/PlacementDriverServer.java (1)
191-191: LGTM - Correctly updated for map-based learner handling
The change from contains() to containsKey() properly aligns with the codebase's transition from list-based to map-based learner handling.
Since this affects the server initialization path, please verify that:
- All configuration parsing tests cover this change
- The server correctly identifies its regions during startup with both regular peers and learners
jraft-core/src/main/java/com/alipay/sofa/jraft/Node.java (4)
20-20: LGTM: Import addition aligns with new map-based learner handling
The addition of the Map import is appropriate for the interface changes.
175-176: Verify implementations of this breaking change
The change from List<PeerId> to Map<PeerId, PeerId> represents a significant architectural shift in learner handling, where each learner is now mapped to its source. This is a breaking change that will require updates to all implementing classes.
Please ensure:
- All implementing classes are updated
- Clients of this interface are modified to handle the new return type
- Migration guide is provided for users of this interface
Also applies to: 182-182
187-188: LGTM: Documentation updates are accurate and consistent
The documentation changes correctly reflect the new method signatures while maintaining appropriate warnings about concurrent modifications.
262-264: Version bump and parameter type change look good
The changes appropriately:
- Bump the version to 1.4.0 for this breaking change
- Update the parameter type to align with the new map-based architecture
However, please verify:
- The version bump is coordinated with the project's release planning
- All existing calls to resetLearners are updated to provide the new Map parameter
jraft-core/src/main/java/com/alipay/sofa/jraft/RouteTable.java (1)
19-19: LGTM: Import addition is appropriate
The addition of java.util.Map import is necessary for the new map-based learner handling implementation.
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/EnumOutter.java (4)
76-77: LGTM: Well-documented enum methods
The JavaDoc additions for EntryType methods are clear and follow best practices, with proper parameter and return value documentation. The deprecation notice correctly guides users to the new forNumber method.
Also applies to: 85-88
204-205: LGTM: Consistent documentation pattern
The JavaDoc additions for ErrorType methods maintain consistency with the EntryType documentation pattern, providing clear guidance for developers.
Also applies to: 213-216
304-305: LGTM: Documentation completeness
The JavaDoc additions for ReadOnlyType methods complete the consistent documentation pattern across all enums in the file.
Also applies to: 313-316
385-386: Verify manual modifications to generated code
While the simplified descriptor initialization looks correct, please verify that this manual modification to the generated protobuf code is safe and won't be overwritten by future protocol buffer compilations. Consider adding a comment explaining why this modification was necessary.
jraft-core/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java (4)
26-26: LGTM: Required import for Map interface
The addition of the Map import is necessary for the new learner management implementation.
120-124: LGTM: Thread-safe learner initialization
The use of ConcurrentHashMap provides thread-safety for learner management, which is appropriate for a cluster implementation.
145-146: LGTM: Proper learner initialization with null source
The use of Configuration.NULL_PEERID as the initial source is appropriate. However, verify that all code paths that read the learner source handle NULL_PEERID correctly.
354-354: LGTM: Proper Map API usage
The change from contains to containsKey correctly adapts to the new Map-based learner storage while maintaining the same logical behavior.
jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java (3)
121-121: LGTM! Good choice of ConcurrentHashMap
The use of ConcurrentHashMap is appropriate here as it provides thread-safe operations without requiring external synchronization.
Also applies to: 125-125
146-147: Verify the semantic meaning of NULL_PEERID
The code uses Configuration.NULL_PEERID as the default value when adding a learner. Please verify:
- Is this the correct default value for a learner without a source?
- Are there any implications for learner behavior when using NULL_PEERID?
355-355: LGTM! Correct adaptation for Map interface
The change from contains() to containsKey() correctly maintains the original logic while adapting to the new Map-based learner storage.
jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java (2)
30-30: LGTM!
The addition of ConcurrentHashMap import aligns with the transition to map-based learner storage.
95-95: LGTM!
The iteration over learners.keySet() correctly adapts to the new map-based structure while maintaining the original functionality.
jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java (4)
29-29: LGTM! Thread-safe collection import added.
The addition of ConcurrentHashMap import aligns with the architectural change to use thread-safe collections for learner management.
83-87: LGTM! Proper initialization of thread-safe learner map.
The learners are correctly initialized using ConcurrentHashMap and properly mapped to NULL_PEERID, maintaining thread safety in the test setup.
95-95: LGTM! Proper iteration over learner map keys.
The iteration correctly uses keySet() to access learner PeerIds from the new map structure.
298-300: LGTM! Snapshot test adapted for map-based learner storage.
The snapshot test correctly iterates over learner PeerIds using the map's keySet().
pom.xml (1)
77-77: Protobuf version update requires verification
The update from protobuf 3.5.1 to 3.8.0 is a good improvement that supports the map-based learner changes and addresses security vulnerabilities. However, this significant version jump requires careful verification:
- Ensure backward compatibility of serialized data
- Verify proto definitions work correctly with the new version
- Test the integration with other dependencies
jraft-core/src/main/java/com/alipay/sofa/jraft/core/FSMCallerImpl.java (1)
633-634: LGTM! Implementation maintains consistency with current configuration handling.
The code correctly mirrors the learner-source mapping approach used for the current configuration, ensuring consistent handling of learners across both current and old configurations.
jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/AddLearnersRequestProcessor.java (2)
21-21: Import statement for Map is necessary and appropriate.
The addition of import java.util.Map; is required for the use of Map<PeerId, PeerId> in the code.
71-72: Ensure consistent logging format and completeness.
The logging statement correctly includes the node ID, remote address, and the learners being added. Verify that addingLearners provides a meaningful string representation to facilitate easier debugging.
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/PeerId.java (5)
55-57: Addition of 'replicationGroup' field is appropriate
The new field replicationGroup is appropriately added with a descriptive comment, enhancing the PeerId class functionality.
84-84: Correctly copying 'replicationGroup' in the copy() method
The copy() method now properly includes the replicationGroup field when creating a new PeerId instance, ensuring all attributes are duplicated.
140-146: Constructor correctly initializes replicationGroup
The constructor properly assigns the replicationGroup field, ensuring that PeerId instances are accurately initialized.
164-166: Getter method for replicationGroup is well-implemented
The getReplicationGroup() method correctly returns the replicationGroup value, providing necessary access to the field.
Line range hint 232-270: Parsing logic correctly supports replicationGroup
The parse() method has been appropriately updated to handle PeerId strings with up to five components, ensuring that replicationGroup is correctly parsed and assigned.
jraft-core/src/main/java/com/alipay/sofa/jraft/conf/Configuration.java (5)
24-24: Approved: Addition of necessary imports
The addition of Map and ConcurrentHashMap imports is appropriate for the changes made in this class.
Also applies to: 26-26
53-53: Ensure thread-safe handling of the learners map
The learners map is a ConcurrentHashMap, suggesting concurrent access. Verify that all modifications to this map are thread-safe and that no additional synchronization is needed.
101-102: Verify null handling for the source parameter in addLearner
In addLearner, there is no null check for the source parameter. Ensure that adding a null source is acceptable or handle null values appropriately to prevent potential NullPointerException.
148-149: Clarify the behavior of copyLearners method
The copyLearners method returns a new ConcurrentHashMap, which might suggest that it's a deep copy. Ensure that the PeerId objects within the map are also copied if necessary to prevent unintended side effects.
111-118:
Handle potential null values in addLearners method
When copying follower, check if it's null before calling follower.copy() to avoid NullPointerException. Consider using NULL_PEERID as a placeholder for null values.
Apply this diff to handle null follower:
for (final Map.Entry<PeerId, PeerId> entry : learners.entrySet()) {
PeerId learner = entry.getKey();
PeerId follower = entry.getValue();
- this.learners.put(learner.copy(), follower == null ? NULL_PEERID : follower.copy());
+ this.learners.put(learner.copy(), follower != null ? follower.copy() : NULL_PEERID);
ret++;
}Likely invalid or redundant comment.
jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java (1)
625-629:
Possible misuse of leaderId when sending requests to followers
In the getLiveLearners method, the GetPeersRequest is constructed with leaderId set to follower.toString() when sending the request to each follower:
.setLeaderId(follower.toString()) // send request to followerHowever, the leaderId field typically should contain the current leader's PeerId, not the follower's. Misusing leaderId might lead to request failures or inconsistent behavior. Please verify the correct usage of the GetPeersRequest and update accordingly.
Consider changing setLeaderId to setPeerId:
- .setLeaderId(follower.toString()) // send request to follower
+ .setPeerId(follower.toString()) // send request to followerjraft-core/src/main/java/com/alipay/sofa/jraft/core/Replicator.java (1)
834-834: Ensure the lock is always released
Adding unlockId(); in the finally block ensures that the lock is released even if an exception occurs during the execution of sendEmptyEntries. This prevents potential deadlocks and ensures proper resource management.
jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/NodeTest.java (1)
792-792: Duplicate of prior comment regarding the use of ConcurrentHashMap
Also applies to: 794-794
jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java (5)
339-351: Ensure Thread-Safe Handling of Learners in ConfigurationCtx
The use of ConcurrentHashMap<PeerId, PeerId> for newLearners and oldLearners requires careful synchronization. Verify that all accesses to these maps are thread-safe to prevent data races.
444-444: Verify Correct Arguments Passed to stopReplicator
When calling stopReplicator(this.newLearners.keySet(), this.oldLearners.keySet());, ensure that the method expects Collection<PeerId>. Verify that passing keySet() of ConcurrentHashMap works as intended without side effects.
Also applies to: 447-447
758-773: Confirm Learners Are Properly Initialized to Avoid NullPointerException
Verify that learnerWithSource is properly initialized and cannot be null before accessing it to prevent potential NullPointerException.
2193-2233: Validate Learner Mapping in fillLogEntryPeers
Ensure that the mapping between learners and their sources is correctly parsed and populated into LogEntry, especially in the conditions handling learnerWithSource and legacy learners without sources.
3286-3291: Defensive Copy in resetLearners for Data Integrity
When resetting learners in resetLearners, using new ConcurrentHashMap<>(learners) is appropriate for a defensive copy. Ensure that the input learners map is not null and is not modified concurrently elsewhere.
jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v2/V2Decoder.java (1)
94-130: Ensure backward compatibility when handling learners
The implementation checks getLearnerWithSourceCount() and falls back to getLearnersCount() when the former is zero. Please verify that this approach correctly handles cases where log entries do not have the learnerWithSource fields, ensuring backward compatibility with existing data.
| System.exit(0); | ||
| } |
There was a problem hiding this comment.
Implement proper cleanup before exit
The program abruptly exits without properly cleaning up resources. The CliService should be shut down gracefully.
- System.exit(0);
+ try {
+ cliService.shutdown();
+ System.exit(0);
+ } catch (InterruptedException e) {
+ System.err.println("Error during shutdown: " + e.getMessage());
+ Thread.currentThread().interrupt();
+ System.exit(1);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| System.exit(0); | |
| } | |
| try { | |
| cliService.shutdown(); | |
| System.exit(0); | |
| } catch (InterruptedException e) { | |
| System.err.println("Error during shutdown: " + e.getMessage()); | |
| Thread.currentThread().interrupt(); | |
| System.exit(1); | |
| } | |
| } |
| Configuration conf = JRaftUtils.getConfiguration("127.0.0.1:8080"); | ||
| List<PeerId> peerIdList = cliService.getPeers(GROUP, conf); | ||
| System.out.println(peerIdList); | ||
| System.exit(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and avoid hardcoded values
The implementation has several areas for improvement:
- Hardcoded configuration values should be externalized
- Missing error handling for potential failures
- The getPeers call should check the returned Status
Consider refactoring like this:
- Configuration conf = JRaftUtils.getConfiguration("127.0.0.1:8080");
- List<PeerId> peerIdList = cliService.getPeers(GROUP, conf);
- System.out.println(peerIdList);
+ String serverAddress = System.getProperty("server.address", "127.0.0.1:8080");
+ Configuration conf = JRaftUtils.getConfiguration(serverAddress);
+
+ Status status = new Status();
+ List<PeerId> peerIdList = cliService.getPeers(GROUP, conf, status);
+ if (!status.isOk()) {
+ System.err.println("Failed to get peers: " + status);
+ System.exit(1);
+ }
+
+ System.out.println("Peers in group " + GROUP + ": " + peerIdList);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Configuration conf = JRaftUtils.getConfiguration("127.0.0.1:8080"); | |
| List<PeerId> peerIdList = cliService.getPeers(GROUP, conf); | |
| System.out.println(peerIdList); | |
| System.exit(0); | |
| String serverAddress = System.getProperty("server.address", "127.0.0.1:8080"); | |
| Configuration conf = JRaftUtils.getConfiguration(serverAddress); | |
| Status status = new Status(); | |
| List<PeerId> peerIdList = cliService.getPeers(GROUP, conf, status); | |
| if (!status.isOk()) { | |
| System.err.println("Failed to get peers: " + status); | |
| System.exit(1); | |
| } | |
| System.out.println("Peers in group " + GROUP + ": " + peerIdList); | |
| System.exit(0); |
| public static void main(String[] args) { | ||
| // 创建并初始化 CliService | ||
| CliService cliService = RaftServiceFactory.createAndInitCliService(new CliOptions()); | ||
| // 使用CliService | ||
| Configuration conf = JRaftUtils.getConfiguration("127.0.0.1:8080"); | ||
| Status status = cliService.removePeer(GROUP, conf, new PeerId("127.0.0.1", 8082)); | ||
| if (status.isOk()) { | ||
| System.out.println("添加节点成功"); | ||
| System.exit(0); | ||
| } | ||
| System.err.println("添加节点失败"); | ||
| System.exit(-1); | ||
| } |
There was a problem hiding this comment.
Multiple implementation issues need to be addressed
The current implementation has several issues that should be fixed:
- Success message contradicts the operation (shows "node added" for remove operation)
- Comments are in Chinese while codebase is in English
- No cleanup of cliService
- Hard-coded values
- Missing error details
- Abrupt System.exit() calls
Here's a suggested improvement:
- public static void main(String[] args) {
- // 创建并初始化 CliService
- CliService cliService = RaftServiceFactory.createAndInitCliService(new CliOptions());
- // 使用CliService
- Configuration conf = JRaftUtils.getConfiguration("127.0.0.1:8080");
- Status status = cliService.removePeer(GROUP, conf, new PeerId("127.0.0.1", 8082));
- if (status.isOk()) {
- System.out.println("添加节点成功");
- System.exit(0);
- }
- System.err.println("添加节点失败");
- System.exit(-1);
- }
+ public static void main(String[] args) {
+ if (args.length != 2) {
+ System.err.println("Usage: TestCli <configPeerAddress> <peerToRemove>");
+ System.exit(1);
+ }
+
+ CliService cliService = null;
+ try {
+ // Initialize CliService
+ cliService = RaftServiceFactory.createAndInitCliService(new CliOptions());
+
+ // Parse configuration and peer to remove
+ Configuration conf = JRaftUtils.getConfiguration(args[0]);
+ PeerId peerToRemove = new PeerId(args[1]);
+
+ // Remove peer from group
+ Status status = cliService.removePeer(GROUP, conf, peerToRemove);
+ if (status.isOk()) {
+ System.out.println("Successfully removed peer: " + peerToRemove);
+ return;
+ }
+
+ System.err.println("Failed to remove peer: " + peerToRemove);
+ System.err.println("Error: " + status.getErrorMsg());
+ } catch (Exception e) {
+ System.err.println("Error: " + e.getMessage());
+ } finally {
+ // Cleanup
+ if (cliService != null) {
+ cliService.shutdown();
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void main(String[] args) { | |
| // 创建并初始化 CliService | |
| CliService cliService = RaftServiceFactory.createAndInitCliService(new CliOptions()); | |
| // 使用CliService | |
| Configuration conf = JRaftUtils.getConfiguration("127.0.0.1:8080"); | |
| Status status = cliService.removePeer(GROUP, conf, new PeerId("127.0.0.1", 8082)); | |
| if (status.isOk()) { | |
| System.out.println("添加节点成功"); | |
| System.exit(0); | |
| } | |
| System.err.println("添加节点失败"); | |
| System.exit(-1); | |
| } | |
| public static void main(String[] args) { | |
| if (args.length != 2) { | |
| System.err.println("Usage: TestCli <configPeerAddress> <peerToRemove>"); | |
| System.exit(1); | |
| } | |
| CliService cliService = null; | |
| try { | |
| // Initialize CliService | |
| cliService = RaftServiceFactory.createAndInitCliService(new CliOptions()); | |
| // Parse configuration and peer to remove | |
| Configuration conf = JRaftUtils.getConfiguration(args[0]); | |
| PeerId peerToRemove = new PeerId(args[1]); | |
| // Remove peer from group | |
| Status status = cliService.removePeer(GROUP, conf, peerToRemove); | |
| if (status.isOk()) { | |
| System.out.println("Successfully removed peer: " + peerToRemove); | |
| return; | |
| } | |
| System.err.println("Failed to remove peer: " + peerToRemove); | |
| System.err.println("Error: " + status.getErrorMsg()); | |
| } catch (Exception e) { | |
| System.err.println("Error: " + e.getMessage()); | |
| } finally { | |
| // Cleanup | |
| if (cliService != null) { | |
| cliService.shutdown(); | |
| } | |
| } | |
| } |
| Arrays.asList(new PeerId("learner", 8082), new PeerId("test", 8182), new PeerId("test", 8183)), | ||
| doneArg.capture()); |
There was a problem hiding this comment.
Test incompatible with new Node interface
According to the changes in the codebase, the Node.addLearners() method now accepts Map<PeerId, PeerId> instead of List<PeerId>. However, this test still verifies the old list-based implementation, which will cause compilation errors.
| learnerWithSource = ctx.node.listLearners(); | ||
| learners = new ArrayList<>(learnerWithSource.keySet()); |
There was a problem hiding this comment.
Add null check for listLearners() result
While the logic is correct, there's a potential NPE risk if ctx.node.listLearners() returns null. Consider adding a null check:
- learnerWithSource = ctx.node.listLearners();
- learners = new ArrayList<>(learnerWithSource.keySet());
+ learnerWithSource = ctx.node.listLearners();
+ learners = learnerWithSource != null ? new ArrayList<>(learnerWithSource.keySet()) : new ArrayList<>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| learnerWithSource = ctx.node.listLearners(); | |
| learners = new ArrayList<>(learnerWithSource.keySet()); | |
| learnerWithSource = ctx.node.listLearners(); | |
| learners = learnerWithSource != null ? new ArrayList<>(learnerWithSource.keySet()) : new ArrayList<>(); |
| @SuppressWarnings("deprecation") | ||
| com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource result = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated valueOf method with forNumber
To avoid deprecation warnings and to use the updated API, replace valueOf(int) with forNumber(int).
Apply this diff to update the method:
- @SuppressWarnings("deprecation")
com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource result = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource
- .valueOf(source_);
+ com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource result = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource
+ .forNumber(source_);Committable suggestion skipped: line range outside the PR's diff.
| @SuppressWarnings("deprecation") | ||
| com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource value = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource | ||
| .valueOf(rawValue); | ||
| if (value == null) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated valueOf method with forNumber
The valueOf(int) method is deprecated; use forNumber(int) instead to adhere to the updated API and avoid deprecation warnings.
Apply this diff to update the method:
- @SuppressWarnings("deprecation")
com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource value = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource
- .valueOf(rawValue);
+ com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource value = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource
+ .forNumber(rawValue);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @SuppressWarnings("deprecation") | |
| com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource value = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource | |
| .valueOf(rawValue); | |
| if (value == null) { | |
| com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource value = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource | |
| .forNumber(rawValue); | |
| if (value == null) { |
| @SuppressWarnings("deprecation") | ||
| com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource result = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated valueOf method with forNumber
The valueOf(int) method is deprecated; using forNumber(int) maintains consistency with the updated enum handling.
Apply this diff to make the change:
- @SuppressWarnings("deprecation")
com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource result = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource
- .valueOf(source_);
+ com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource result = com.alipay.sofa.jraft.entity.LocalFileMetaOutter.FileSource
+ .forNumber(source_);Committable suggestion skipped: line range outside the PR's diff.
| for (PeerId learner : learners) { | ||
| PeerId sourcePeer = getTargetSourcePeer(learner.getReplicationGroup()); | ||
| if (sourcePeer == null) { | ||
| throw new JRaftException("can not get target source peer for new learner: " + learner); | ||
| } | ||
| newConf.addLearner(learner, sourcePeer); | ||
| } | ||
| unsafeRegisterConfChange(this.conf.getConf(), newConf, done); | ||
| } finally { | ||
| this.writeLock.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Exception with Error Handling in addLearners Method
Throwing an unchecked JRaftException when a target source peer cannot be determined may lead to an application crash. Instead, consider returning an error via the Closure to allow graceful error handling.
Apply this diff to replace the exception with error handling:
public void addLearners(final List<PeerId> learners, final Closure done) {
checkPeers(learners);
this.writeLock.lock();
try {
final Configuration newConf = new Configuration(this.conf.getConf());
for (PeerId learner : learners) {
PeerId sourcePeer = getTargetSourcePeer(learner.getReplicationGroup());
- if (sourcePeer == null) {
- throw new JRaftException("can not get target source peer for new learner: " + learner);
- }
+ if (sourcePeer == null) {
+ if (done != null) {
+ done.run(new Status(RaftError.EINTERNAL, "Cannot determine source peer for learner: " + learner));
+ }
+ return;
+ }
newConf.addLearner(learner, sourcePeer);
}
unsafeRegisterConfChange(this.conf.getConf(), newConf, done);
} finally {
this.writeLock.unlock();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (PeerId learner : learners) { | |
| PeerId sourcePeer = getTargetSourcePeer(learner.getReplicationGroup()); | |
| if (sourcePeer == null) { | |
| throw new JRaftException("can not get target source peer for new learner: " + learner); | |
| } | |
| newConf.addLearner(learner, sourcePeer); | |
| } | |
| unsafeRegisterConfChange(this.conf.getConf(), newConf, done); | |
| } finally { | |
| this.writeLock.unlock(); | |
| } | |
| } | |
| for (PeerId learner : learners) { | |
| PeerId sourcePeer = getTargetSourcePeer(learner.getReplicationGroup()); | |
| if (sourcePeer == null) { | |
| if (done != null) { | |
| done.run(new Status(RaftError.EINTERNAL, "Cannot determine source peer for learner: " + learner)); | |
| } | |
| return; | |
| } | |
| newConf.addLearner(learner, sourcePeer); | |
| } | |
| unsafeRegisterConfChange(this.conf.getConf(), newConf, done); | |
| } finally { | |
| this.writeLock.unlock(); | |
| } | |
| } |
| addReplicatorStateListener(new Replicator.ReplicatorStateListener() { | ||
| @Override | ||
| public void onCreated(PeerId peer) { | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public void onError(PeerId peer, Status status) { | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public void onDestroyed(PeerId peer) { | ||
| // if follower destroyed, transfer learner to other node | ||
| List<PeerId> learners = new ArrayList<>(); | ||
| Map<PeerId, PeerId> learnerWithSource = getCurrentConf().getLearners(); | ||
| for (Map.Entry<PeerId, PeerId> entry : learnerWithSource.entrySet()) { | ||
| if (peer.equals(entry.getValue())) { | ||
| learners.add(entry.getKey()); | ||
| } | ||
| } | ||
| if (learners.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| LOG.info("Transfer learners to another node because Node {} fails, learners: {}", peer, learners); | ||
| addLearners(learners, new Closure() { | ||
| @Override | ||
| public void run(Status status) { | ||
| if (status.isOk()) { | ||
| LOG.info("Finish transfer learners to another node because Node {} fails, learners: {}", | ||
| peer, learners); | ||
| } else { | ||
| LOG.error( | ||
| "Failed to transfer learners to another node because Node {} fails, status: {}, learners: {}", | ||
| peer, status, learners); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Exceptions in ReplicatorStateListener Methods
Ensure that exceptions in the onDestroyed method are properly handled to prevent silent failures.
Consider wrapping the logic in a try-catch block:
public void onDestroyed(PeerId peer) {
+ try {
// existing logic
List<PeerId> learners = new ArrayList<>();
Map<PeerId, PeerId> learnerWithSource = getCurrentConf().getLearners();
for (Map.Entry<PeerId, PeerId> entry : learnerWithSource.entrySet()) {
if (peer.equals(entry.getValue())) {
learners.add(entry.getKey());
}
}
if (learners.isEmpty()) {
return;
}
LOG.info("Transfer learners to another node because Node {} fails, learners: {}", peer, learners);
addLearners(learners, new Closure() {
@Override
public void run(Status status) {
if (status.isOk()) {
LOG.info("Finished transferring learners to another node because Node {} failed, learners: {}", peer, learners);
} else {
LOG.error("Failed to transfer learners to another node because Node {} failed, status: {}, learners: {}", peer, status, learners);
}
}
});
+ } catch (Exception e) {
+ LOG.error("Exception in onDestroyed for peer {}: {}", peer, e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addReplicatorStateListener(new Replicator.ReplicatorStateListener() { | |
| @Override | |
| public void onCreated(PeerId peer) { | |
| } | |
| @Override | |
| public void onError(PeerId peer, Status status) { | |
| } | |
| @Override | |
| public void onDestroyed(PeerId peer) { | |
| // if follower destroyed, transfer learner to other node | |
| List<PeerId> learners = new ArrayList<>(); | |
| Map<PeerId, PeerId> learnerWithSource = getCurrentConf().getLearners(); | |
| for (Map.Entry<PeerId, PeerId> entry : learnerWithSource.entrySet()) { | |
| if (peer.equals(entry.getValue())) { | |
| learners.add(entry.getKey()); | |
| } | |
| } | |
| if (learners.isEmpty()) { | |
| return; | |
| } | |
| LOG.info("Transfer learners to another node because Node {} fails, learners: {}", peer, learners); | |
| addLearners(learners, new Closure() { | |
| @Override | |
| public void run(Status status) { | |
| if (status.isOk()) { | |
| LOG.info("Finish transfer learners to another node because Node {} fails, learners: {}", | |
| peer, learners); | |
| } else { | |
| LOG.error( | |
| "Failed to transfer learners to another node because Node {} fails, status: {}, learners: {}", | |
| peer, status, learners); | |
| } | |
| } | |
| }); | |
| } | |
| addReplicatorStateListener(new Replicator.ReplicatorStateListener() { | |
| @Override | |
| public void onCreated(PeerId peer) { | |
| } | |
| @Override | |
| public void onError(PeerId peer, Status status) { | |
| } | |
| @Override | |
| public void onDestroyed(PeerId peer) { | |
| try { | |
| // if follower destroyed, transfer learner to other node | |
| List<PeerId> learners = new ArrayList<>(); | |
| Map<PeerId, PeerId> learnerWithSource = getCurrentConf().getLearners(); | |
| for (Map.Entry<PeerId, PeerId> entry : learnerWithSource.entrySet()) { | |
| if (peer.equals(entry.getValue())) { | |
| learners.add(entry.getKey()); | |
| } | |
| } | |
| if (learners.isEmpty()) { | |
| return; | |
| } | |
| LOG.info("Transfer learners to another node because Node {} fails, learners: {}", peer, learners); | |
| addLearners(learners, new Closure() { | |
| @Override | |
| public void run(Status status) { | |
| if (status.isOk()) { | |
| LOG.info("Finish transfer learners to another node because Node {} fails, learners: {}", | |
| peer, learners); | |
| } else { | |
| LOG.error( | |
| "Failed to transfer learners to another node because Node {} fails, status: {}, learners: {}", | |
| peer, status, learners); | |
| } | |
| } | |
| }); | |
| } catch (Exception e) { | |
| LOG.error("Exception in onDestroyed for peer {}: {}", peer, e); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
jraft-extension/rpc-grpc-impl/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java (2)
91-99: Consider adding documentation for the Map structure.The change from LinkedHashSet to Map is good, but it would be helpful to document the meaning of the key-value pairs in the learners map (e.g., what PeerId represents the key vs. value).
135-135: Consider documenting the NULL_PEERID usage.The change to use Map.put with Configuration.NULL_PEERID is correct, but adding a comment explaining why NULL_PEERID is used would improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java(4 hunks)jraft-extension/rpc-grpc-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java(4 hunks)jraft-extension/rpc-grpc-impl/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java(5 hunks)
🔇 Additional comments (11)
jraft-extension/rpc-grpc-impl/src/test/java/com/alipay/sofa/jraft/core/TestCluster.java (3)
27-27: LGTM!
The added Map import is necessary for the new learners data structure.
110-114: LGTM!
Good choice using ConcurrentHashMap for thread safety in the default constructor, and the parameter type update maintains consistency with the new learners structure.
342-342: LGTM!
The update to use containsKey() correctly adapts the follower filtering logic to work with the new Map-based learners structure.
jraft-extension/rpc-grpc-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java (4)
29-29: LGTM!
The addition of ConcurrentHashMap import aligns with the transition to map-based learner management.
82-85: LGTM! Setup correctly implements map-based learner management.
The changes properly:
- Initialize learners as a ConcurrentHashMap
- Use Configuration.NULL_PEERID as the default value
- Update iteration to use keySet() for backward compatibility
Also applies to: 93-94
160-160: LGTM! Test properly adapted for map-based learners.
The test correctly retrieves learners from the map's keySet while maintaining the original test logic.
282-283: LGTM! Snapshot test properly adapted for map-based learners.
The test correctly iterates over learners using the map's keySet while maintaining the original snapshot testing logic.
jraft-extension/java-log-storage-impl/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java (4)
29-29: LGTM: Import added for new map-based learner management
The addition of ConcurrentHashMap import aligns with the transition to map-based learner management.
82-86: LGTM: Setup correctly implements map-based learner management
The changes appropriately:
- Use ConcurrentHashMap for thread-safe learner management
- Map each learner to NULL_PEERID as a placeholder
- Update iteration to use keySet() for compatibility
Also applies to: 94-94
161-161: LGTM: Test correctly adapted to use map-based learner access
The change properly retrieves learners using keySet() from the new map-based structure.
283-284: LGTM: Snapshot test correctly updated for map-based learners
The iteration over learners has been properly updated to use getLearners().keySet(), maintaining consistency with the new data structure.
Motivation:
Explain the context, and why you're making that change.
To make others understand what is the problem you're trying to solve.
Modification:
Describe the idea and modifications you've done.
Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests