Replication Snapshot and Compact #368

yamingk · 2024-04-06T01:04:21Z

Reviewers please focus on resource mgr, home raft repl log, raft repl dev and JournalVDev.
Not much logic change in other layer/files.

Changes:

add snapshot creation to listener (HomeObject PR is in progress) and compact handling (called by raft and in repl dev it will persist compact_lsn as a barrier for truncation).
Note: this is not a generic snapshot that we will implement for other use cases (e.g. HomeBlks). This snapshot creation is bear minimum for nuraft snapshot and HomeObject use case.
add home raft log store truncation. The truncation will be triggered by nuraft server after a snapshot is created.
in HomeStore it remembers the compact_lsn and later resource manager will trigger all logstore truncation in the system.
HomeStore doesn't reserve any extra log items. Reservation is done through dynamic config which used to initialize raft server on startup.
add resource manager resource timer to trigger raft log store truncation as needed.
Add JournalVDev critical high watermark, in its callback it will trigger raft log store truncation immediately so that it doesn't need to want for next timer audit.

Testing:

snapshot creation and truncation can be triggered from the logs with below command line.
./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
it also needs to be added to replication long duration test.

codecov-commenter · 2024-04-06T01:26:38Z

Codecov Report

Attention: Patch coverage is 36.90476% with 53 lines in your changes are missing coverage. Please review.

Project coverage is 57.84%. Comparing base (60808d0) to head (deb52b8).

Files	Patch %	Lines
src/lib/replication/repl_dev/raft_repl_dev.cpp	0.00%	15 Missing ⚠️
src/lib/common/resource_mgr.cpp	62.85%	10 Missing and 3 partials ⚠️
.../lib/replication/log_store/home_raft_log_store.cpp	0.00%	13 Missing ⚠️
src/lib/replication/log_store/repl_log_store.cpp	0.00%	4 Missing ⚠️
src/lib/replication/repl_dev/raft_repl_dev.h	0.00%	4 Missing ⚠️
...rc/lib/replication/repl_dev/raft_state_machine.cpp	0.00%	2 Missing ⚠️
src/lib/checkpoint/cp_mgr.cpp	0.00%	1 Missing ⚠️
src/lib/replication/service/raft_repl_service.cpp	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   57.76%   57.84%   +0.07%     
==========================================
  Files         108      107       -1     
  Lines        9519     9563      +44     
  Branches     1233     1231       -2     
==========================================
+ Hits         5499     5532      +33     
- Misses       3481     3493      +12     
+ Partials      539      538       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/include/homestore/replication/repl_dev.h

src/lib/common/resource_mgr.cpp

sanebay · 2024-04-09T17:54:10Z

src/lib/logstore/log_store_service.cpp

@@ -106,7 +106,7 @@ void LogStoreService::start(bool format) {
 }

 void LogStoreService::stop() {
-    device_truncate(nullptr, true, false);
+    // device_truncate(nullptr, true, false);


So device_truncate() is called only from resource mgr. Should we call even if there is no resource crunch issue ?

this device_truncate is calling into log_dev to do actual truncate, we should not do that without setting the compact_lsn in logstore (through nuraft::compact) because only nuraft knows which should be the correct compact_lsn. We already persisted the compact_lsn in superblock, in reboot, it can catch up in next timer schedule or next critical alert is hit. I don't quite get what is the 'crunch issue' here in your comment.

src/lib/replication/log_store/home_raft_log_store.cpp

sanebay · 2024-04-11T18:53:12Z

src/lib/common/resource_mgr.cpp

+        res_mgr_timer_ms * 1000 * 1000, true /* recurring */, nullptr /* cookie */, iomgr::reactor_regex::all_worker,
+        [this](void*) {
+            // all resource timely audit routine should arrive here;
+            this->trigger_truncate();


The periodic execution is affecting UT too. Should we have some condition of vdev size more than some threshold, then only trigger the truncate.

Can you specify affecting by how? Sorry I probably didn't catch the question. Right now the threshold we have is the critical one, and this one is to a batch truncate for all the log stores in homestore system wide.

there are some UT mostly on test_journal_vdev.cpp carefully verify the journal offset/size etc in a controlled manner (append some log, check, append some logs, check, truncate, check).

I think the concern here is if the time based truncation introduced, UT follows this pattern is not possible.

This is the reason we didn't reserve at log_dev, but to let nuraft do the reserve is because we don't want to make changes in log_dev layer, because that will break a lot of assumptions the current UT does in test_journal_vdev and test_log_dev.

I spent more a week on that journey and finally realized to do the reserve in logdev (crying out loud), need to basically redesign all the UTs in those layer...

In summary, the change in this PR won't break any assumption in layer below log store. Multiple truncation during multiple timer event won't cause any side effect (even though no snapshot/compact in between), because they are protected by safe_truncation_boundary which is conducted by compact_lsn also (only source in nuraft::compact call)

Concern is min(last - reserved, companct_lsn), if nuraft havent called compact, compact_lsn is 0 and periodic timer will truncate all entries

src/include/homestore/replication_service.hpp

src/lib/common/homestore_config.fbs

src/lib/common/resource_mgr.cpp

xiaoxichen · 2024-04-16T17:16:56Z

src/lib/common/resource_mgr.cpp

+        res_mgr_timer_ms * 1000 * 1000, true /* recurring */, nullptr /* cookie */, iomgr::reactor_regex::all_worker,
+        [this](void*) {
+            // all resource timely audit routine should arrive here;
+            this->trigger_truncate();


there are some UT mostly on test_journal_vdev.cpp carefully verify the journal offset/size etc in a controlled manner (append some log, check, append some logs, check, truncate, check).

I think the concern here is if the time based truncation introduced, UT follows this pattern is not possible.

src/lib/common/resource_mgr.cpp

src/lib/common/resource_mgr.hpp

xiaoxichen · 2024-04-16T17:51:42Z

src/lib/device/journal_vdev.cpp

+#endif
+
+    // high watermark check for the entire journal vdev;
+    if (resource_mgr().check_journal_vdev_size(m_vdev.used_size(), m_vdev.size())) {


as the m_event_cb is nullptr , how this expect to link to truncation?

it is via m_journal_vdev_exceed_cb registered from here: https://github.com/yamingk/HomeStore/blob/yk_repl_new/src/lib/device/journal_vdev.cpp#L58

Oh ok, it is a bit wired as the code path here seems like

if ( DETECTION)) { update_metrics do callback to trigger action }

however the m_event_cb is nullptr and the actual callback is done in the detection part((resource_mgr().check_journal_vdev_size) .

Can I read this as your TODO to move to m_event_cb?

Yes. The goal is in resource mgr to move all layer_spcecif_registered_cb to general event_cb. Here in this journal vdev, I think the only consumer of log store and we can see if there is still any use case this event_cb is still needed since most of the job is done via resource manager.

src/lib/common/resource_mgr.hpp

src/lib/logstore/log_store.cpp

src/lib/replication/log_store/home_raft_log_store.cpp

…pl_new

xiaoxichen

lgtm

src/lib/common/homestore_config.fbs

sanebay · 2024-04-18T22:06:23Z

src/lib/common/resource_mgr.cpp

+        res_mgr_timer_ms * 1000 * 1000, true /* recurring */, nullptr /* cookie */, iomgr::reactor_regex::all_worker,
+        [this](void*) {
+            // all resource timely audit routine should arrive here;
+            this->trigger_truncate();


Concern is min(last - reserved, companct_lsn), if nuraft havent called compact, compact_lsn is 0 and periodic timer will truncate all entries

src/tests/test_raft_repl_dev.cpp

yamingk and others added 17 commits March 19, 2024 11:52

issue 258: replication truncate initial commit

78eb8e5

issue 258: replication truncate initial commit

b7f2739

fix

458973e

homestore truncate

794114d

update api documents

d8b8209

issue 258: replication truncate initial commit

fd91b26

issue 258: replication truncate initial commit

6eff25a

homestore truncate

edf64e6

update api documents

d45a17b

add test case

38bbace

resolve conflict

a64d938

fix log and comments

cfeb2fb

add more api docs

ce5c3d4

add last_snapshot() api

697dba8

nuraft to drive compact

efe261e

bump conan ver

3a06119

Merge branch 'master' into yk_repl_new

deb52b8

sanebay reviewed Apr 9, 2024

View reviewed changes

sanebay reviewed Apr 11, 2024

View reviewed changes

Merge branch 'master' into yk_repl_new

98b1322

xiaoxichen reviewed Apr 16, 2024

View reviewed changes

yamingk and others added 5 commits April 16, 2024 20:38

address comments

4cff991

Merge branch 'yk_repl_new' of github.com:yamingk/HomeStore into yk_re…

8addac2

…pl_new

Merge branch 'master' into yk_repl_new

bcbbe2d

add comment around num resvd log items

e19a105

Merge branch 'yk_repl_new' of github.com:yamingk/HomeStore into yk_re…

46d2aaf

…pl_new

xiaoxichen approved these changes Apr 18, 2024

View reviewed changes

sanebay reviewed Apr 18, 2024

View reviewed changes

yamingk merged commit 6ae31e1 into eBay:master Apr 18, 2024
21 checks passed

yamingk deleted the yk_repl_new branch May 14, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replication Snapshot and Compact #368

Replication Snapshot and Compact #368

yamingk commented Apr 6, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024

sanebay Apr 9, 2024

yamingk Apr 15, 2024

sanebay Apr 11, 2024

yamingk Apr 15, 2024

xiaoxichen Apr 16, 2024

yamingk Apr 16, 2024 •

edited

Loading

sanebay Apr 18, 2024

xiaoxichen Apr 16, 2024

xiaoxichen Apr 16, 2024

yamingk Apr 16, 2024

xiaoxichen Apr 17, 2024 •

edited

Loading

yamingk Apr 17, 2024 •

edited

Loading

xiaoxichen left a comment

sanebay Apr 18, 2024

Replication Snapshot and Compact #368

Replication Snapshot and Compact #368

Conversation

yamingk commented Apr 6, 2024 • edited Loading

Changes:

Testing:

codecov-commenter commented Apr 6, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamingk Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxichen Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

yamingk Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

xiaoxichen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamingk commented Apr 6, 2024 •

edited

Loading

yamingk Apr 16, 2024 •

edited

Loading

xiaoxichen Apr 17, 2024 •

edited

Loading

yamingk Apr 17, 2024 •

edited

Loading