-
Notifications
You must be signed in to change notification settings - Fork 22
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
Replication Snapshot and Compact #364
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #364 +/- ##
==========================================
- Coverage 61.26% 57.73% -3.53%
==========================================
Files 135 107 -28
Lines 14361 9547 -4814
Branches 1727 1229 -498
==========================================
- Hits 8798 5512 -3286
+ Misses 4834 3499 -1335
+ Partials 729 536 -193 ☔ View full report in Codecov by Sentry. |
@@ -86,6 +89,8 @@ class RaftReplDev : public ReplDev, | |||
|
|||
RaftReplDevMetrics m_metrics; | |||
|
|||
nuraft::ptr< nuraft::snapshot > m_last_snapshot{nullptr}; |
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.
Why do homestore need to maintain the last snapshot. Isn't upper layer going to maintain snapshot. It could potentially store multiple snapshots?
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.
You point is valid. I also thought about it, I did this only because raft is asking for a nuraft::snapshot and we don't want to expose to upper layer, and in this snapshot, there is only log_idx and log_term which is not mutable to upper layer (right?), so I didn't ask for upper layer to get the last snapshot.
Raft server only asks about last snapshot (to call corresponding compact), upper layer can very well store multiple "snapshots" as it needs which is transparent to lower layer.
void RaftReplDev::on_create_snapshot(nuraft::snapshot& s, nuraft::async_result< bool >::handler_type& when_done) { | ||
RD_LOG(DEBUG, "create_snapshot last_idx={}/term={}", s.get_last_log_idx(), s.get_last_log_term()); | ||
repl_snapshot snapshot{.last_log_idx_ = s.get_last_log_idx(), .last_log_term_ = s.get_last_log_term()}; | ||
auto result = m_listener->on_create_snapshot(snapshot).get(); |
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.
I think we need to ask upper layer to create the snapshot and require upper layer snapshot to be derived from repl_snapshot and store required parameters like last_term and last_log_idx. This way how snapshot created and maintained is completely controlled by upper layer.
Changes:
Reviewers please focus on resource mgr, home raft repl log, raft repl dev and JournalVDev.
Not much logic change in other layer/files.
Testing:
./bin/test_raft_repl_dev --gtest_filter=Snapshot_and_Compact --log_mods replication:debug --num_io=999999 --snapshot_distance=200 --num_raft_logs_resv=20000 --res_mgr_audit_timer_ms=120000