feat: distinguish between normal node startup and snapshot loading #319
Conversation
panlei-coder
changed the title
|
@panlei-coder please fix ci failure and file confliction. |
ok |
pikiwidb.conf
Outdated
| @@ -343,6 +343,6 @@ rocksdb-ttl-second 604800 | |||
| rocksdb-periodic-second 259200; | |||
|
|
|||
| ############################### RAFT ############################### | |||
| use-raft yes | |||
| use-raft no | |||
There was a problem hiding this comment.
想的是默认单机场景下不启用 raft，而使用 rocksdb 的 wal，不过这个需要提 pr，我记个 issue 吧，看有谁愿意领一下
…i-coder/pikiwidb into new_log_playback_to_latest
WalkthroughThe recent updates encompass modifications to various CMake files, configuration files, and source code to enhance functionality, fix bugs, and optimize performance. Key changes include updating external project repositories, adjusting OpenSSL paths, modifying snapshot handling in RaftNodeCmd, and adding new methods for managing snapshots and log indices in the PRaft and Storage classes. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant RaftNodeCmd
participant PRAFT
participant Storage
Client->>RaftNodeCmd: Request DoCmdSnapshot
RaftNodeCmd->>Storage: GetSmallestFlushedLogIndex
Storage-->>RaftNodeCmd: Return self_snapshot_index
RaftNodeCmd->>PRAFT: Call DoSnapshot(self_snapshot_index)
PRAFT-->>RaftNodeCmd: Return status
RaftNodeCmd->>Client: SetRes(CmdRes::kOK)
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 Configration 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: 4
Outside diff range and nitpick comments (1)
src/praft/psnapshot.cc (1)
Line range hint
69-89: The snapshot generation logic is comprehensive. However, consider replacing the hardcodedlast_log_indexwith a dynamically retrieved or configured value to avoid potential issues in different environments.- auto last_log_index = 30000; // @todo + auto last_log_index = PSTORE.GetBackend(db_id)->GetStorage()->GetSmallestFlushedLogIndex(); // Dynamic retrieval
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- cmake/braft.cmake (1 hunks)
- cmake/openssl.cmake (2 hunks)
- pikiwidb.conf (1 hunks)
- save_load.sh (2 hunks)
- src/CMakeLists.txt (1 hunks)
- src/cmd_raft.cc (1 hunks)
- src/db.cc (1 hunks)
- src/praft/praft.cc (5 hunks)
- src/praft/praft.h (2 hunks)
- src/praft/psnapshot.cc (3 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/log_index.cc (2 hunks)
- src/storage/src/redis.h (1 hunks)
- src/storage/src/storage.cc (1 hunks)
Files skipped from review due to trivial changes (3)
- cmake/openssl.cmake
- pikiwidb.conf
- src/CMakeLists.txt
Additional context used
Shellcheck
save_load.sh
[info] 6-6: Use ./glob or -- glob so names with dashes won't become options. (SC2035)
[info] 7-7: Use ./glob or -- glob so names with dashes won't become options. (SC2035)
Additional comments not posted (12)
save_load.sh (1)
14-16: The addition of repeatedredis-benchmarkandredis-clicommands appears to be for stress testing or functionality checks.cmake/braft.cmake (1)
19-23: The change in GIT repository and tag points to a different branch for testing. Ensure that this is intended for permanent integration or revert after testing.src/db.cc (1)
40-43: Switching fromstd::bindto a lambda expression fordo_snapshot_functionenhances readability and performance. Good use of modern C++ practices.src/praft/psnapshot.cc (1)
Line range hint
69-89: Handling of files in the snapshot directory is appropriately managed to ensure only relevant files are considered.src/praft/praft.h (1)
140-141: The addition of new methods and a member variable supports new functionalities and enhances the RAFT module's capabilities. Good use of default arguments for backward compatibility.Also applies to: 170-170
src/storage/src/log_index.cc (1)
172-172: The addition of logging statements enhances system visibility and aids in debugging and monitoring. Good practice.src/storage/src/redis.h (1)
113-115: Implementation ofGetSmallestFlushedLogIndexlooks correct.This method correctly retrieves the smallest flushed log index, which is crucial for ensuring that the node's state is consistent with the persisted state.
src/praft/praft.cc (3)
245-252: Added methodsGetTermandGetLastLogIndexenhance the functionality of the PRaft class by providing access to specific log details.Also applies to: 254-261
659-682: The logic inon_snapshot_loadhandles different startup scenarios effectively. However, ensure that the methodGetSmallestFlushedLogIndexis implemented correctly and efficiently.Verification successful
The method
GetSmallestFlushedLogIndexis indeed implemented in the codebase. Here are the relevant locations:
- Declaration:
src/storage/include/storage/storage.h- Implementation:
src/storage/src/storage.ccThis confirms that the method is implemented and used correctly.
src/storage/include/storage/storage.h: LogIndex GetSmallestFlushedLogIndex() const;src/storage/src/storage.cc: LogIndex Storage::GetSmallestFlushedLogIndex() const {Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `GetSmallestFlushedLogIndex`. # Test: Search for the implementation and usage of `GetSmallestFlushedLogIndex`. Expect: Efficient and correct implementation. ast-grep --lang cpp --pattern $'class Storage { $$$ LogIndex GetSmallestFlushedLogIndex() const { $$$ } $$$ }' src/storage/include/storage/storage.hLength of output: 494
Script:
#!/bin/bash # Description: Verify the implementation of `GetSmallestFlushedLogIndex`. # Test: Search for the implementation and usage of `GetSmallestFlushedLogIndex`. Expect: Efficient and correct implementation. # Refine the search pattern and broaden the search scope. ast-grep --lang cpp --pattern $'LogIndex GetSmallestFlushedLogIndex() const { $$$ }'Length of output: 85
Script:
#!/bin/bash # Description: Verify the implementation of `GetSmallestFlushedLogIndex`. # Test: Search for the implementation and usage of `GetSmallestFlushedLogIndex`. Expect: Efficient and correct implementation. # Broaden the search to include potential variations of the method name and search across all files. rg --glob '!*.md' 'GetSmallestFlushedLogIndex'Length of output: 869
154-154: Ensure the path construction uses secure and efficient string handling.src/storage/include/storage/storage.h (1)
1126-1127: Add documentation forGetSmallestFlushedLogIndex.It's good practice to document public methods, especially for complex functionalities like this one. Consider adding a brief comment explaining what "smallest flushed log index" refers to and how it should be used.
src/storage/src/storage.cc (1)
2520-2527: The implementation ofGetSmallestFlushedLogIndexmethod is correct and efficient.
| // @todo need to get self_snapshot_index | ||
| // auto self_snapshot_index = PSTORE.GetBackend(client->GetCurrentDB())->GetStorage()->GetSmallestFlushedLogIndex(); | ||
| auto s = PRAFT.DoSnapshot(); // self_snapshot_index |
There was a problem hiding this comment.
Complete the implementation for self_snapshot_index or clarify the TODO.
The commented-out code suggests that self_snapshot_index is intended to be used but is not implemented. Would you like assistance in completing this part of the code?
| @@ -9,6 +9,7 @@ | |||
|
|
|||
| #include <cassert> | |||
|
|
|||
| #include "braft/raft.h" | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process did not find any direct or indirect usage of entities from braft/raft.h in src/praft/praft.cc. The references to raft found were in comments and error messages, which do not necessitate the inclusion of the header file.
- Lines: 12-12
- File:
src/praft/praft.cc
Analysis chain
Ensure the newly added #include "braft/raft.h" is necessary.
Let's further investigate if there are any indirect usages or dependencies that might require the inclusion of braft/raft.h.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the newly added include is used in the file.
# Test: Search for usage of entities from `braft/raft.h`. Expect: At least one usage.
ast-grep --lang cpp --pattern $'$_ { $$$ braft::raft::$$$ }' src/praft/praft.cc
Length of output: 80
Script:
#!/bin/bash
# Description: Verify if there are any indirect usages or dependencies that might require the inclusion of `braft/raft.h`.
# Test 1: Search for any usage of `braft` namespace in the file.
ast-grep --lang cpp --pattern $'$_ { $$$ braft::$$$ }' src/praft/praft.cc
# Test 2: Search for any usage of `raft` keyword in the file.
rg --word-regexp 'raft' src/praft/praft.cc
Length of output: 343
save_load.sh
Outdated
| @@ -1,4 +1,5 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
Tip
Codebase Verification
The save_load.sh script contains glob patterns (rm -fr *) that could potentially be affected by filenames with dashes. To prevent filenames with dashes from being interpreted as options, consider using ./* instead of *.
- Line 2:
rm -fr *should be updated torm -fr ./ * - Line 7:
rm -fr *should be updated torm -fr ./ *
Analysis chain
Consider using ./ or -- with glob patterns to prevent filenames with dashes from being interpreted as options.
Also applies to: 7-7
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if filenames with dashes are being interpreted as options.
# Test: Check if filenames with dashes exist and could potentially be interpreted as options.
find . -name "*-*" -exec echo "Filename with dash found: {}" \;
Length of output: 3925
Script:
#!/bin/bash
# Description: Check for glob patterns in save_load.sh that could be affected by filenames with dashes.
# Test: Search for glob patterns in save_load.sh
grep -E '\*|\?' save_load.sh
Length of output: 217
src/praft/psnapshot.cc
Outdated
| #include "braft/snapshot.h" | ||
| #include "butil/files/file_path.h" | ||
|
|
||
| #include "pstd/log.h" | ||
| #include "pstd/pstd_string.h" | ||
|
|
||
| #include "config.h" | ||
| #include "praft.h" |
There was a problem hiding this comment.
The additions and changes to snapshot path parsing are necessary for the new functionality. Consider adding unit tests to cover potential edge cases in the parsing logic.
Would you like me to help in creating unit tests for the snapshot path parsing logic?
Also applies to: 31-51
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
1.方案：
（1）如果数据库正常关闭，理论上是不需要进行日志回放的，因为数据库在关闭是会将内存数据刷盘，并将日志截断到最新的位置，这样就可以保证正常关闭下的节点启动是no-op的。那么在数据库非正常关闭时，节点启动如果加载了快照，只会是一个多余的操作，因为快照本身就是空的，只有snapshot_meta数据。所以，在节点启动的时候，第一次调用on_snapshot_load接口理论上是不需要进行快照的加载，需要对第一个启动和正常快照安装进行区分。但是有一点需要注意，节点在刚加入集群且没有数据时，它在初始化之后是不会加载本地的快照（因为快照文件夹是空的），所以在安装来自 leader 的快照时需要 LoadDBFromCheckpoint 过程。
（2）节点在启动的时候，它是按照快照中 snapshot_meta 记录的 last_applied_index 作为日志回放点，那么在故障重启时，节点需要过滤掉一些日志，这无疑是影响效率的。我们可以在节点调用 on_load_snapshot 开始位置就设置这个日志回放点的位置，根据 DB 中最小 flush-index 来设置（这里的逻辑需要进一步的完善）。
（3）braft 的 pr : pikiwidb/braft#4
2.测试
因为获取 DB 中最小的 flush-index 目前还没有完善，所以在测试的时候将一个 DB 下 rocksdb 的数量设置成了1，使用save_load.sh 脚本进行测试。
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Refactor
std::bindwith lambda expressions for snapshot functions.Chores
sleepcommand in initialization script.