feat: supports the master-slave mode #342
Conversation
|
Warning Rate limit exceeded@panlei-coder has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 28 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. Walkthrough最近的更改主要集中在增强 Raft 集群管理功能。关键更新包括修改 CMake 中的构建配置、更新 Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin
participant InfoCmd as InfoCmd
participant RaftNode as RaftNode
participant RaftCluster as RaftCluster
participant PRaft as PRaft
Admin->>InfoCmd: 请求 Raft 信息
InfoCmd->>PRaft: 获取领导者 ID
PRaft-->>InfoCmd: 返回领导者 ID
InfoCmd->>PRaft: 获取节点信息
PRaft-->>InfoCmd: 返回节点信息
InfoCmd-->>Admin: 显示 Raft 信息
Admin->>RaftCluster: 添加节点（学习者）
RaftCluster->>PRaft: 添加 Peer（学习者）
PRaft-->>RaftCluster: 确认添加
RaftCluster-->>Admin: 节点已添加
Admin->>RaftCluster: 移除节点（学习者）
RaftCluster->>PRaft: 移除 Peer（学习者）
PRaft-->>RaftCluster: 确认移除
RaftCluster-->>Admin: 节点已移除
Poem
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 as PR comments)
Additionally, you can add 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: 7
Outside diff range and nitpick comments (4)
src/cmd_raft.cc (2)
Line range hint
154-198: Review the cluster joining logic for error handling and message clarity.The
ClusterJoinfunction could improve its error messages for better clarity and debugging. Specifically, the message for an already initialized node could be more descriptive.- "A node that has been added to a cluster must be removed from the old cluster before it can be added to the new cluster" + "Node initialization error: Please remove the node from its current cluster before adding it to a new one."
Line range hint
237-253: Ensure proper validation of IP and port in theDoCmdJoinfunction.The function
DoCmdJoinuses theGetIpAndPortFromEndPointutility to parse the IP and port but does not handle the case where the IP or port might be invalid. Consider adding error handling for this scenario.+ if (!ip_port.has_value()) { + return client->SetRes(CmdRes::kErrOther, "Invalid IP or port provided."); + }src/praft/praft.cc (2)
Line range hint
37-48: Ensure proper synchronization and error handling inClusterCmdContext::Set.The method
ClusterCmdContext::Setmodifies several member variables under a mutex lock, which is good for thread safety. However, there's no error handling ifclient_is notnullptrwhen this method is called, which could lead to silent logical errors if the method is misused. Consider adding an error log or exception in this case.
291-294: Clarify the role of thelearnerflag inPRaft::SendNodeAddRequest.The method
PRaft::SendNodeAddRequestappends "learner" to the request ifis_learneris true. It would be beneficial for maintainability to add a comment explaining why this distinction is necessary, especially for future developers or for debugging.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- CMakeLists.txt (1 hunks)
- cmake/braft.cmake (1 hunks)
- src/base_cmd.h (2 hunks)
- src/cmd_admin.cc (1 hunks)
- src/cmd_raft.cc (7 hunks)
- src/cmd_raft.h (1 hunks)
- src/cmd_table_manager.cc (1 hunks)
- src/praft/praft.cc (9 hunks)
- src/praft/praft.h (4 hunks)
- src/pstd/pstd_string.cc (2 hunks)
- src/pstd/pstd_string.h (2 hunks)
- tests/consistency_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- CMakeLists.txt
Additional context used
golangci-lint
tests/consistency_test.go
27-27: undefined: redis (typecheck)
28-28: undefined: redis (typecheck)
31-31: undefined: Describe (typecheck)
819-819: undefined: redis (typecheck)
37-37: undefined: BeforeAll (typecheck)
44-44: undefined: Expect (typecheck)
49-49: undefined: Expect (typecheck)
50-50: undefined: Expect (typecheck)
80-80: undefined: AfterAll (typecheck)
90-90: undefined: BeforeEach (typecheck)
105-105: undefined: AfterEach (typecheck)
117-117: undefined: It (typecheck)
156-156: undefined: It (typecheck)
189-189: undefined: It (typecheck)
Additional comments not posted (11)
src/cmd_raft.h (1)
83-107: Addition ofMasterCmdandSlaveofCmdclasses to handle master-slave commands.Consider adding unit tests to ensure the functionality of these new commands.
src/pstd/pstd_string.h (1)
101-103: Addition ofIsValidIPandIsValidPortfunctions to validate IP addresses and ports.Consider adding unit tests to ensure the robustness of these new utility functions.
src/praft/praft.h (1)
Line range hint
54-142: Modifications and additions to handle learners in the Raft cluster.Ensure thorough testing, especially integration tests, to verify the correct handling of learners in various cluster scenarios.
src/cmd_table_manager.cc (1)
73-74: Addition ofMasterandSlaveofcommands to the command table with specific priorities.Ensure testing to verify that these commands are correctly integrated and functional within the system.
src/cmd_admin.cc (1)
198-226: Updates toInfoCmd::InfoRaftfunction to improve the display of Raft node information.Consider adding comments or documentation to explain these changes for the benefit of future maintainers.
src/cmd_raft.cc (2)
87-99: Validate the number of arguments for theRAFT.NODE ADDcommand.The validation logic for the number of arguments in the
DoCmdAddfunction is correctly implemented, ensuring that either 4 or 5 arguments are provided. This is crucial for maintaining the command's integrity.
118-136: Check the role type handling in theRAFT.NODE REMOVEcommand.The handling of the role type (learner or follower) in the
DoCmdRemovefunction appears correct, but ensure that this aligns with the expected behavior in all use cases.Verification successful
The implementation of the
DoCmdRemovefunction insrc/cmd_raft.cccorrectly handles the role type (learner or follower) for theRAFT.NODE REMOVEcommand. The function checks the number of arguments and sets theis_learnerflag based on the fourth argument if provided.
src/cmd_raft.cclines 118-136: The role type handling in theDoCmdRemovefunction aligns with the expected behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the role type handling in RAFT.NODE REMOVE command. # Test: Search for the function usage. Expect: Correct handling of role types. rg --type python $'DoCmdRemove'Length of output: 61
Script:
#!/bin/bash # Description: Verify the role type handling in RAFT.NODE REMOVE command. # Search for the definition of the DoCmdRemove function in C++ files. rg --type cpp 'DoCmdRemove' # Search for the usage of the DoCmdRemove function in C++ files. rg --type cpp 'DoCmdRemove('Length of output: 448
Script:
#!/bin/bash # Description: Extract the implementation of the DoCmdRemove function from src/cmd_raft.cc. # Extract the implementation of the DoCmdRemove function. ast-grep --lang cpp --pattern $'void DoCmdRemove(PClient* client) { $$$ }' src/cmd_raft.ccLength of output: 93
Script:
#!/bin/bash # Description: Extract the implementation of the DoCmdRemove function from src/cmd_raft.cc. # Search for the implementation of the DoCmdRemove function. rg --type cpp --context 20 'void DoCmdRemove(PClient* client)' src/cmd_raft.ccLength of output: 80
Script:
#!/bin/bash # Description: Extract the implementation of the DoCmdRemove function from src/cmd_raft.cc. # Search for the implementation of the DoCmdRemove function with increased context lines. rg --type cpp --context 40 'DoCmdRemove' src/cmd_raft.ccLength of output: 6018
src/base_cmd.h (1)
45-46: Addition of new command constants for master-slave mode.The constants
kCmdNameMasterandkCmdNameSlaveofhave been correctly added to support the new master-slave mode functionality. This is a necessary update for the new feature.src/pstd/pstd_string.cc (1)
662-674: Validate IP address and port number utility functions.The functions
IsValidIPandIsValidPortare correctly implemented to validate IP addresses and port numbers, respectively. These functions are crucial for ensuring the validity of network addresses in the application.src/praft/praft.cc (2)
306-309: Ensure consistency in handling thelearnerflag inPRaft::SendNodeRemoveRequest.The method
PRaft::SendNodeRemoveRequestalso appends "learner" based on theis_learnerflag. Verify that this behavior is consistent and correctly documented across all methods that use this flag.
161-161: Validate the use ofis_learnerflag inPRaft::Init.The
is_learnerflag is set based on theClusterCmdContext. Ensure that this flag is appropriately used throughout the system and that its value is consistent with the expected behavior in all contexts where it's accessed.Verification successful
To ensure the
is_learnerflag is used consistently and correctly across the system, we need to verify its initialization and usage in the relevant methods and contexts. Let's start by examining the initialization and setting of theis_learnerflag inpraft.ccand its usage in methods likeAddPeerandRemovePeer.
To ensure the
is_learnerflag is used consistently and correctly across the system, we need to re-run the verification scripts with adjusted patterns to ensure they match the code correctly.
The previous attempts to verify the usage of the
is_learnerflag did not produce any output, which suggests that the patterns used might not have matched the code correctly. To ensure a thorough verification, we need to adjust our approach and re-run the verification scripts with more precise patterns.
The verification of the
is_learnerflag usage across the system has been successful. The flag is initialized and set correctly inpraft.cc, and its usage in methods likeAddPeerandRemovePeeris consistent with the expected behavior. Additionally, the flag is appropriately used in theClusterRemoveandClusterJoinfunctions incmd_raft.cc.
is_learneris initialized insrc/praft/praft.h:bool is_learner_ = false;Usage in
AddPeermethod insrc/praft/praft.cc:butil::Status PRaft::AddPeer(const std::string& peer, bool is_learner) { if (!node_) { ERROR_LOG_AND_STATUS("Node is not initialized"); } braft::SynchronizedClosure done; if (is_learner) { node_->add_learner(peer, &done); } else { node_->add_peer(peer, &done); } }Usage in
RemovePeermethod insrc/praft/praft.cc:butil::Status PRaft::RemovePeer(const std::string& peer, bool is_learner) { if (!node_) { return ERROR_LOG_AND_STATUS("Node is not initialized"); } braft::SynchronizedClosure done; if (is_learner) { node_->remove_learner(peer, &done); } else { node_->remove_peer(peer, &done); } }Usage in
ClusterRemovefunction insrc/cmd_raft.cc:static void ClusterRemove(PClient* client, bool is_learner) { // Function implementation }Usage in
ClusterJoinfunction insrc/cmd_raft.cc:static void ClusterJoin(PClient* client, std::string&& ip, int port, bool is_learner) { // Function implementation }The
is_learnerflag is used consistently and correctly across the system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `is_learner` flag across the system. # Test: Search for the usage of `is_learner`. Expect: Consistent and correct usage across the system. rg --type cpp 'is_learner'Length of output: 1719
Script:
#!/bin/bash # Verify the initialization and setting of `is_learner` in `praft.cc` ast-grep --lang cpp --pattern $'bool is_learner_ = $_;' src/praft/praft.cc # Verify the usage of `is_learner` in `AddPeer` and `RemovePeer` methods in `praft.cc` ast-grep --lang cpp --pattern $'butil::Status PRaft::AddPeer(const std::string& peer, bool is_learner) { $$$ }' src/praft/praft.cc ast-grep --lang cpp --pattern $'butil::Status PRaft::RemovePeer(const std::string& peer, bool is_learner) { $$$ }' src/praft/praft.cc # Verify the usage of `is_learner` in `cmd_raft.cc` ast-grep --lang cpp --pattern $'static void ClusterRemove(PClient* client, bool is_learner) { $$$ }' src/cmd_raft.cc ast-grep --lang cpp --pattern $'static void ClusterJoin(PClient* client, std::string&& ip, int port, bool is_learner) { $$$ }' src/cmd_raft.ccLength of output: 612
Script:
#!/bin/bash # Verify the initialization and setting of `is_learner` in `praft.cc` ast-grep --lang cpp --pattern 'bool is_learner_ = $_;' src/praft/praft.cc # Verify the usage of `is_learner` in `AddPeer` and `RemovePeer` methods in `praft.cc` ast-grep --lang cpp --pattern 'butil::Status PRaft::AddPeer(const std::string& peer, bool is_learner) { $$$ }' src/praft/praft.cc ast-grep --lang cpp --pattern 'butil::Status PRaft::RemovePeer(const std::string& peer, bool is_learner) { $$$ }' src/praft/praft.cc # Verify the usage of `is_learner` in `cmd_raft.cc` ast-grep --lang cpp --pattern 'static void ClusterRemove(PClient* client, bool is_learner) { $$$ }' src/cmd_raft.cc ast-grep --lang cpp --pattern 'static void ClusterJoin(PClient* client, std::string&& ip, int port, bool is_learner) { $$$ }' src/cmd_raft.ccLength of output: 612
Script:
#!/bin/bash # Verify the initialization and setting of `is_learner` in `praft.cc` rg --type cpp 'bool is_learner_ =' # Verify the usage of `is_learner` in `AddPeer` and `RemovePeer` methods in `praft.cc` rg --type cpp 'butil::Status PRaft::AddPeer' -A 10 rg --type cpp 'butil::Status PRaft::RemovePeer' -A 10 # Verify the usage of `is_learner` in `cmd_raft.cc` rg --type cpp 'static void ClusterRemove' -A 10 rg --type cpp 'static void ClusterJoin' -A 10Length of output: 2722
| func getPeerID(client *redis.Client, ctx context.Context) (string, bool) { | ||
| info, err := client.Do(ctx, "info", "raft").Result() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| info_str := info.(string) | ||
| scanner := bufio.NewScanner(strings.NewReader(info_str)) | ||
| var peer_id string | ||
| var is_member bool | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.Contains(line, "raft_peer_id") { | ||
| parts := strings.Split(line, ":") | ||
| if len(parts) >= 2 { | ||
| peer_id = parts[1] | ||
| is_member = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return peer_id, is_member | ||
| } |
There was a problem hiding this comment.
Ensure the redis package is correctly imported to resolve the 'undefined: redis' error.
+ import "github.com/redis/go-redis/v9"Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
819-819: undefined: redis (typecheck)
| // raft | ||
| for _, follower := range followers { | ||
| info, err := follower.Do(ctx, "info", "raft").Result() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| info_str := info.(string) | ||
| scanner := bufio.NewScanner(strings.NewReader(info_str)) | ||
| var peer_id string | ||
| var is_member bool | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.Contains(line, "raft_peer_id") { | ||
| parts := strings.Split(line, ":") | ||
| if len(parts) >= 2 { | ||
| peer_id = parts[1] | ||
| is_member = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
| peer_id, is_member := getPeerID(follower, ctx) | ||
|
|
||
| if is_member { | ||
| ret, err := follower.Do(ctx, "raft.node", "remove", peer_id).Result() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(ret).To(Equal(OK)) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| for _, f := range followers { | ||
| ret, err := f.Do(ctx, "slaveof", "127.0.0.1", "12111").Result() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(ret).To(Equal(OK)) | ||
| } | ||
|
|
||
| time.Sleep(10 * time.Second) | ||
|
|
||
| for _, f := range followers { | ||
| ret, err := f.Do(ctx, "slaveof", "no", "one").Result() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(ret).To(Equal(OK)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Ensure the It function from the Ginkgo testing framework is correctly imported to resolve the 'undefined: It' error.
+ import . "github.com/onsi/ginkgo/v2"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.
| // raft | |
| for _, follower := range followers { | |
| info, err := follower.Do(ctx, "info", "raft").Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| info_str := info.(string) | |
| scanner := bufio.NewScanner(strings.NewReader(info_str)) | |
| var peer_id string | |
| var is_member bool | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| if strings.Contains(line, "raft_peer_id") { | |
| parts := strings.Split(line, ":") | |
| if len(parts) >= 2 { | |
| peer_id = parts[1] | |
| is_member = true | |
| break | |
| } | |
| } | |
| } | |
| peer_id, is_member := getPeerID(follower, ctx) | |
| if is_member { | |
| ret, err := follower.Do(ctx, "raft.node", "remove", peer_id).Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(ret).To(Equal(OK)) | |
| } | |
| } | |
| }) | |
| for _, f := range followers { | |
| ret, err := f.Do(ctx, "slaveof", "127.0.0.1", "12111").Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(ret).To(Equal(OK)) | |
| } | |
| time.Sleep(10 * time.Second) | |
| for _, f := range followers { | |
| ret, err := f.Do(ctx, "slaveof", "no", "one").Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(ret).To(Equal(OK)) | |
| } | |
| }) | |
| + import . "github.com/onsi/ginkgo/v2" | |
| // raft | |
| for _, follower := range followers { | |
| peer_id, is_member := getPeerID(follower, ctx) | |
| if is_member { | |
| ret, err := follower.Do(ctx, "raft.node", "remove", peer_id).Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(ret).To(Equal(OK)) | |
| } | |
| } | |
| for _, f := range followers { | |
| ret, err := f.Do(ctx, "slaveof", "127.0.0.1", "12111").Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(ret).To(Equal(OK)) | |
| } | |
| time.Sleep(10 * time.Second) | |
| for _, f := range followers { | |
| ret, err := f.Do(ctx, "slaveof", "no", "one").Result() | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(ret).To(Equal(OK)) | |
| } | |
| }) |
cmake/braft.cmake
Outdated
| GIT_REPOSITORY https://github.com/panlei-coder/braft.git # "https://github.com/pikiwidb/braft.git" | ||
| GIT_TAG fix_learner_bug # master |
There was a problem hiding this comment.
Tip
Codebase Verification
The temporary changes to GIT_REPOSITORY and GIT_TAG are still present in the cmake/braft.cmake file.
GIT_REPOSITORY https://github.com/panlei-coder/braft.gitGIT_TAG fix_learner_bug
Ensure to revert these changes once the permanent fix is merged into the main braft repository.
Analysis chain
Temporary change to GIT_REPOSITORY and GIT_TAG to address a specific issue.
Ensure to revert these changes once the permanent fix is merged into the main braft repository.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the temporary changes to GIT_REPOSITORY and GIT_TAG have been reverted.
# Test: Search for the temporary repository and tag in the cmake file. Expect: No occurrences after the fix is merged.
rg --type cmake $'GIT_REPOSITORY https://github.com/panlei-coder/braft.git|GIT_TAG fix_learner_bug' cmake/braft.cmake
Length of output: 266
| static void ClusterRemove(PClient* client, bool is_learner) { | ||
| // Get the leader information | ||
| braft::PeerId leader_peer_id(PRAFT.GetLeaderID()); | ||
| // @todo There will be an unreasonable address, need to consider how to deal with it | ||
| if (leader_peer_id.is_empty()) { | ||
| client->SetRes(CmdRes::kErrOther, | ||
| "The leader address of the cluster is incorrect, try again or delete the node from another node"); | ||
| return; | ||
| } | ||
|
|
||
| // Connect target | ||
| std::string peer_ip = butil::ip2str(leader_peer_id.addr.ip).c_str(); | ||
| auto port = leader_peer_id.addr.port - pikiwidb::g_config.raft_port_offset; | ||
| auto peer_id = client->argv_[2]; | ||
| auto ret = PRAFT.GetClusterCmdCtx().Set(ClusterCmdType::kRemove, client, std::move(peer_ip), port, is_learner, | ||
| std::move(peer_id)); | ||
| if (!ret) { // other clients have removed | ||
| return client->SetRes(CmdRes::kErrOther, "Other clients have removed"); | ||
| } | ||
| PRAFT.GetClusterCmdCtx().ConnectTargetNode(); | ||
| INFO("Sent remove request to leader successfully"); | ||
|
|
||
| // Not reply any message here, we will reply after the connection is established. | ||
| client->Clear(); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling when the leader's address is incorrect.
The function ClusterRemove attempts to handle an incorrect leader address by setting an error response, but it does not log this significant event. Consider adding a log statement before returning to aid in debugging.
+ INFO("Incorrect leader address detected.");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.
| static void ClusterRemove(PClient* client, bool is_learner) { | |
| // Get the leader information | |
| braft::PeerId leader_peer_id(PRAFT.GetLeaderID()); | |
| // @todo There will be an unreasonable address, need to consider how to deal with it | |
| if (leader_peer_id.is_empty()) { | |
| client->SetRes(CmdRes::kErrOther, | |
| "The leader address of the cluster is incorrect, try again or delete the node from another node"); | |
| return; | |
| } | |
| // Connect target | |
| std::string peer_ip = butil::ip2str(leader_peer_id.addr.ip).c_str(); | |
| auto port = leader_peer_id.addr.port - pikiwidb::g_config.raft_port_offset; | |
| auto peer_id = client->argv_[2]; | |
| auto ret = PRAFT.GetClusterCmdCtx().Set(ClusterCmdType::kRemove, client, std::move(peer_ip), port, is_learner, | |
| std::move(peer_id)); | |
| if (!ret) { // other clients have removed | |
| return client->SetRes(CmdRes::kErrOther, "Other clients have removed"); | |
| } | |
| PRAFT.GetClusterCmdCtx().ConnectTargetNode(); | |
| INFO("Sent remove request to leader successfully"); | |
| // Not reply any message here, we will reply after the connection is established. | |
| client->Clear(); | |
| } | |
| static void ClusterRemove(PClient* client, bool is_learner) { | |
| // Get the leader information | |
| braft::PeerId leader_peer_id(PRAFT.GetLeaderID()); | |
| // @todo There will be an unreasonable address, need to consider how to deal with it | |
| if (leader_peer_id.is_empty()) { | |
| INFO("Incorrect leader address detected."); | |
| client->SetRes(CmdRes::kErrOther, | |
| "The leader address of the cluster is incorrect, try again or delete the node from another node"); | |
| return; | |
| } | |
| // Connect target | |
| std::string peer_ip = butil::ip2str(leader_peer_id.addr.ip).c_str(); | |
| auto port = leader_peer_id.addr.port - pikiwidb::g_config.raft_port_offset; | |
| auto peer_id = client->argv_[2]; | |
| auto ret = PRAFT.GetClusterCmdCtx().Set(ClusterCmdType::kRemove, client, std::move(peer_ip), port, is_learner, | |
| std::move(peer_id)); | |
| if (!ret) { // other clients have removed | |
| return client->SetRes(CmdRes::kErrOther, "Other clients have removed"); | |
| } | |
| PRAFT.GetClusterCmdCtx().ConnectTargetNode(); | |
| INFO("Sent remove request to leader successfully"); | |
| // Not reply any message here, we will reply after the connection is established. | |
| client->Clear(); | |
| } |
CMakeLists.txt
Outdated
| @@ -33,7 +33,7 @@ IF(CMAKE_BUILD_TYPE STREQUAL "Release") | |||
| MESSAGE("Building in Release mode") | |||
| ELSE() | |||
| MESSAGE("Building in Debug mode") | |||
| OPTION(ADDRESS_SANITIZER "Enable AddressSanitizer (default sanitizer)" ON) | |||
| OPTION(ADDRESS_SANITIZER "Enable AddressSanitizer (default sanitizer)" OFF) | |||
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
src/cmd_raft.cc (1)
26-50: 改进ClusterRemove函数新增的
ClusterRemove函数封装了节点移除逻辑，改进了代码组织和可重用性。然而，它缺少对重要事件的日志记录。建议在处理错误和成功发送请求时添加日志记录，以便调试和监控。
+ INFO("Attempting to remove node from cluster."); ... + INFO("Incorrect leader address detected."); ... + INFO("Sent remove request to leader successfully.");
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/cmd_admin.cc (1 hunks)
- src/cmd_raft.cc (7 hunks)
- src/praft/praft.cc (9 hunks)
- src/praft/praft.h (4 hunks)
- tests/consistency_test.go (1 hunks)
Additional context used
Learnings (1)
src/cmd_admin.cc (1)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.cc:248-289 Timestamp: 2024-07-10T14:45:11.380Z Learning: The error messages in `InfoCmd::InfoRaft` were updated to use more standard English for improved readability and professionalism.
Additional comments not posted (26)
src/praft/praft.h (10)
55-55: 新增参数is_learner
Set方法新增了bool is_learner参数，这使得可以区分学习者和非学习者节点。这一改动与支持主从模式的 PR 目标一致。
63-63: 添加const限定符
GetClusterCmdType方法添加了const限定符，确保该方法不会修改对象的状态，提高了代码的安全性和清晰度。
64-64: 添加const限定符
GetClient方法添加了const限定符，确保该方法不会修改对象的状态，提高了代码的安全性和清晰度。
65-65: 添加const限定符
GetPeerIp方法添加了const限定符，确保该方法不会修改对象的状态，提高了代码的安全性和清晰度。
66-66: 添加const限定符
GetPort方法添加了const限定符，确保该方法不会修改对象的状态，提高了代码的安全性和清晰度。
67-67: 添加const限定符
GetPeerID方法添加了const限定符，确保该方法不会修改对象的状态，提高了代码的安全性和清晰度。
68-68: 新增方法IsLearner新增的
IsLearner方法提供了对is_learner_成员变量的访问，这对于新功能是必要的。
108-108: 新增参数is_learner
AddPeer方法新增了bool is_learner参数，这使得可以区分学习者和非学习者节点。这一改动与支持主从模式的 PR 目标一致。
109-109: 新增参数is_learner
RemovePeer方法新增了bool is_learner参数，这使得可以区分学习者和非学习者节点。这一改动与支持主从模式的 PR 目标一致。
143-143: 新增方法GetListLearners新增的
GetListLearners方法增强了功能，允许检索学习者节点，这是新主从模式所必需的。src/cmd_admin.cc (1)
197-225: 改进InfoRaft方法
InfoRaft方法更新了获取 leader ID 的方式，使用了PRAFT.GetLeaderID()，并新增了报告普通节点和学习者节点的逻辑。这些改动提高了 Raft 状态报告的清晰度和完整性。src/cmd_raft.cc (7)
87-103: 改进DoCmdAdd方法
DoCmdAdd方法更新后接受一个可选的is_learner参数，使其在添加节点时能够区分学习者和跟随者角色。这些改动提高了命令的灵活性。
118-138: 改进DoCmdRemove方法
DoCmdRemove方法更新后在当前节点不是 leader 时使用新的ClusterRemove函数，并接受一个可选的is_learner参数。这些改动简化了移除过程，并集中处理节点移除请求的逻辑。
Line range hint
156-181: 新增ClusterInit函数新增的
ClusterInit函数封装了集群初始化逻辑，改进了代码组织。
182-200: 新增ClusterJoin函数新增的
ClusterJoin函数封装了加入集群的逻辑，改进了代码组织。
Line range hint
239-255: 改进DoCmdJoin方法
DoCmdJoin方法更新后调用新的ClusterJoin函数，简化了命令结构。
302-304: 改进DoCmdJoin方法
DoCmdJoin方法更新后调用新的ClusterJoin函数，并将is_learner设置为 true，确保SlaveofCmd类在加入集群时正确处理学习者节点。
306-324: 改进DoCmdRemove方法
DoCmdRemove方法更新后调用新的ClusterRemove函数，并将is_learner设置为 true，确保SlaveofCmd类在从集群中移除时正确处理学习者节点。src/praft/praft.cc (6)
Line range hint
37-48:
更改已批准：添加了is_learner参数。此更改正确地将新参数
is_learner分配给类成员is_learner_，与方法的其余部分一致。
161-161: 更改已批准：在PRaft::Init方法中设置is_learner。此更改正确地将学习者状态集成到节点初始化过程中。
310-313: 更改已批准：在PRaft::SendNodeAddRequest方法中处理学习者节点。此更改通过在请求中添加 "learner" 关键字来正确处理学习者节点的情况。
325-328: 更改已批准：在PRaft::SendNodeRemoveRequest方法中处理学习者节点。此更改通过在请求中添加 "learner" 关键字来正确处理学习者节点的情况。
522-532: 更改已批准：在PRaft::AddPeer方法中处理学习者节点。此更改通过在
is_learner为 true 时调用add_learner方法来正确处理学习者节点的添加。
543-553: 更改已批准：在PRaft::RemovePeer方法中处理学习者节点。此更改通过在
is_learner为 true 时调用remove_learner方法来正确处理学习者节点的移除。tests/consistency_test.go (2)
792-816: 更改已批准：在ThreeNodesClusterConstructionTest测试用例中添加了设置和取消从节点的逻辑。这些新命令对于测试集群在不同配置下的行为至关重要，改进了一致性测试的健壮性。
819-839: 更改已批准：添加了getPeerID辅助函数。此函数通过减少测试代码中的冗余，提高了代码的可读性和可维护性。
src/praft/praft.cc
Outdated
| butil::Status PRaft::GetListLearners(std::vector<braft::PeerId>* learners) { | ||
| if (!node_) { | ||
| ERROR_LOG_AND_STATUS("Node is not initialized"); | ||
| } | ||
|
|
||
| return node_->list_learners(learners); | ||
| } |
There was a problem hiding this comment.
改进错误处理：在 PRaft::GetListLearners 方法中返回更具描述性的错误状态。
当前方法在节点未初始化时返回默认构造的 butil::Status 对象，这可能无法清楚地向调用者传达错误状态。建议返回更具描述性的错误状态。
- ERROR_LOG_AND_STATUS("Node is not initialized");
+ return butil::Status(EINVAL, "Node is not initialized");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.
| butil::Status PRaft::GetListLearners(std::vector<braft::PeerId>* learners) { | |
| if (!node_) { | |
| ERROR_LOG_AND_STATUS("Node is not initialized"); | |
| } | |
| return node_->list_learners(learners); | |
| } | |
| butil::Status PRaft::GetListLearners(std::vector<braft::PeerId>* learners) { | |
| if (!node_) { | |
| return butil::Status(EINVAL, "Node is not initialized"); | |
| } | |
| return node_->list_learners(learners); | |
| } |
先 close 掉，后面再考虑 master-slave mode |
Close it first and consider master-slave mode later. |
issue: #211
需要先合并 pikiwidb/braft#6 这个 pr。
由于该 pr 需要修改 braft 部分的源码，所以在 braft.cmake 中先改成了我自己的仓库，等后续合并了 braft 对应的 pr 之后再改回来。
主从模式的实现大体的逻辑和 raft 集群的逻辑一致，具体的命令如下：
主从
master：master init
slave：slaveof ip port （learner） （如果检测到自己是followr，则先移除集群，再以learner的方式加入）
slaveof no one 解除主从关系
braft
leader：raft.cluster init
follower：raft.cluster join ip:port （如果检测到自己是learner，则先解除主从关系，再重新加入集群）
raft.cluster remove peer_id
Summary by CodeRabbit
Summary by CodeRabbit
新功能
改进
重构
错误修复