Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: distinguish between normal node startup and snapshot loading #319
feat: distinguish between normal node startup and snapshot loading #319
Changes from 9 commits
dc6c861
610c28c
0e452d3
689eaa2
e38513c
42d7d0a
3f8ae41
dcdf232
6f9f6fb
e46d213
929a5db
8e56daf
deedd3c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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*
.rm -fr *
should be updated torm -fr ./ *
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:
Length of output: 3925
Script:
Length of output: 217
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议:在 Lambda 表达式中使用值捕获。
使用引用捕获
raft
可能导致潜在的生命周期问题,尤其是在raft
对象的生命周期可能比storage_options
短的场景中。建议改为值捕获或确保raft
的生命周期总是足够长。Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The verification process did not find any direct or indirect usage of entities from
braft/raft.h
insrc/praft/praft.cc
. The references toraft
found were in comments and error messages, which do not necessitate the inclusion of the header 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:
Length of output: 80
Script:
Length of output: 343
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
兔子 留言说这个头文件没用到,却引入了,请确认
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexStocks 经过验证,
#include "braft/raft.h"
头文件在src/praft/praft.cc
中没有被直接使用。仅在注释和错误消息中出现了raft
关键字。因此,可以安全地移除这个头文件。Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 424
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node_->set_self_playback_point(replay_point+1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里 set_self_playback_point 设置的 last_applied_index,所以就是 replay_point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如上所示,把 uint64_t 改为 LogIndex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议:为快照路径解析逻辑添加单元测试
快照路径解析逻辑对于新功能是必要的。建议添加单元测试以覆盖潜在的边界情况。
需要我帮助创建快照路径解析逻辑的单元测试吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这行是不是忘换了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯 忘记换了