feat: add current_fork_perc in info all command #2640

kostasrim · 2024-02-21T18:22:22Z

add field current_snapshot_perc
add field current_save_keys_processed
add field current_save_keys_total

adiholden · 2024-02-22T07:02:45Z

src/server/server_family.cc

-        append("save_buffer_bytes", save_controller_->GetSaveBuffersSize());
-      }
+    if (has_save_controller) {
+      append("save_buffer_bytes", save_controller_->GetSaveBuffersSize());


you need to lock save_mu_ before accessing save_controller_

omg 😱 😱 good catch!

adiholden · 2024-02-22T07:10:20Z

src/server/server_state.h

+    size_t keys_total = 0;
+  };
+
+  CurrentSaveStats current_save_stats;


why dont you put this stats in SliceSnapshot class and add a funciton in SaveStagesController that aggregates the stats as we do in GetSaveBuffersSize

if you have a new function GetSaveProgress you will not need the save_mode_ as well. you can always set the counters and reply them in info persistance only if save_controller_ exists

The thing is that we need save_mode_ because otherwise a new SliceSnapshot (that is not part of the bgsave/save operation) might overwrite the thread local. We exercise almost the same path in replication, so it could be the case that replication and a bgsave operation happen at the same time. Both of the two command paths will happily increment the thread locals messing with the correctness.

Second is that with what I have here, we save a hop. We don't dispatch a cb to fetch and aggregate the thread locals. We are in non critical path so this is less important but still

I am suggesting to add the stats to SliceSnapshot class, not to use the thread local

I did the change and to be honest I am not very happy with it:

It's more code and introduces a way more convoluted code path

I needed 3 different proxy functions to reach the SliceSnapshot 😢

code repetition because we need to handle explicitly different formats/requirements

but it's fine for now, but at some point I would consider a refactor. It's too much work to plug in stat tracking. That 3 proxy call in GetSCurrentSnapshotProgress killed it for me

I agree there are lots of proxy functions in this flow and it looks messy
we can check later if we can do some refactoring here to make it simpler

adiholden · 2024-02-22T07:13:38Z

src/server/server_family.cc

+      total = 0;
+    }
+
+    append("current_fork_perc", perc);


I believe we can change the name here, use different one than redis as we dont have a fork

I agree I just opted in for consistency instead of adding a different name. I will rename accordingly

adiholden · 2024-02-22T13:23:28Z

src/server/server_family.h

@@ -99,6 +99,8 @@ struct Metrics {
  // Max length of the all the tx shard-queues.
  uint32_t tx_queue_len = 0;

+  ServerState::CurrentSaveStats current_save_stats;


not used anymore

already removed :)

adiholden · 2024-02-22T13:27:09Z

src/server/snapshot.cc

@@ -360,4 +364,8 @@ size_t SliceSnapshot::GetTotalChannelCapacity() const {
  return dest_->GetSize();
 }

+std::pair<size_t, size_t> SliceSnapshot::GetCurrentSnapshotProgress() const {
+  return {stats_.loop_serialized, stats_.keys_total};


side_saved + loop_serialized
is the number of entries serialized

nice catch!

adiholden · 2024-02-22T13:33:00Z

src/server/server_family.cc

+    lock_guard lk{save_mu_};
+    if (save_controller_) {
+      has_save_controller = true;
+      save_buffer_bytes = save_controller_->GetSaveBuffersSize();


I think each of this calls should be inside the relevant if otherwise you invoke the functions even if the user didnt ask for memory/persistence stats

but now we need to lock twice.... And we also now in the worst path, do an extra two hops.... The added overhead and complexity is piling up...

you also already have below a lock for save_mu_ for
curent_durration_sec = save_controller_->GetCurrentSaveDuration();
I think its better to have them inside the relevant path
You are doing the hops right now regardless of calling the mutex once or twice

and also one more comment here. Let's zoom out, from one side, we are saying here that we care about the potential two extra dispatches to the shards and the cost associated with it but at the same time we don't care that we added two extra shard dispatches (for GetCurrentSnapshotProgress) when in fact we did not need them in the first place..

Anyway i made the change -- just thinking out loud :)

There is only one hop for GetCurrentSnapshotProgress right? not 2. Or I did not understand what you mean..

adiholden · 2024-02-22T13:38:16Z

src/server/detail/save_stages_controller.cc

@@ -129,6 +131,11 @@ size_t RdbSnapshot::GetSaveBuffersSize() {
  return saver_->GetTotalBuffersSize();
 }

+std::pair<size_t, size_t> RdbSnapshot::GetCurrentSnapshotProgress() const {


How about returning a struct SnapshotStats, it will be more clear to reader and you wont need to add a comment what is the first and second param

kostasrim · 2024-02-22T16:50:21Z

test failure is fixed by #2647

adiholden · 2024-02-25T07:53:06Z

src/server/server_family.cc

+    {
+      lock_guard lk{save_mu_};
+      if (save_controller_) {
+        auto res = save_controller_->GetCurrentSnapshotProgress();


you can move this to line 1967 right? we already have this code
lock_guard lk{save_mu_};
if (save_controller_) {
there

feat: add current_fork_perc in info all command

a2e794e

kostasrim force-pushed the save_progress_tracking branch from e415e88 to a2e794e Compare February 21, 2024 18:23

kostasrim self-assigned this Feb 21, 2024

adiholden reviewed Feb 22, 2024

View reviewed changes

kostasrim added 5 commits February 22, 2024 11:50

Merge branch 'main' into save_progress_tracking

16dfc19

address gh comments

62fe530

address gh comments

8031ecb

remove unused code

aea3d96

remove unused comments

72afcbb

kostasrim requested a review from adiholden February 22, 2024 13:14

remove unused code

b71d3c5

adiholden reviewed Feb 22, 2024

View reviewed changes

address gh comments

71c541a

adiholden reviewed Feb 25, 2024

View reviewed changes

kostasrim added 2 commits February 26, 2024 10:06

Merge branch 'main' into save_progress_tracking

d70bd61

address gh comments

91b3b51

kostasrim requested a review from adiholden February 26, 2024 08:31

adiholden approved these changes Feb 26, 2024

View reviewed changes

kostasrim merged commit d54f220 into main Feb 26, 2024
10 checks passed

kostasrim deleted the save_progress_tracking branch February 26, 2024 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add current_fork_perc in info all command #2640

feat: add current_fork_perc in info all command #2640

kostasrim commented Feb 21, 2024 •

edited

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

adiholden Feb 22, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

kostasrim Feb 22, 2024

kostasrim Feb 22, 2024

adiholden Feb 25, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024 •

edited

adiholden Feb 22, 2024

kostasrim Feb 22, 2024 •

edited

adiholden Feb 25, 2024

adiholden Feb 22, 2024

kostasrim Feb 22, 2024

kostasrim commented Feb 22, 2024

adiholden Feb 25, 2024

feat: add current_fork_perc in info all command #2640

feat: add current_fork_perc in info all command #2640

Conversation

kostasrim commented Feb 21, 2024 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Feb 22, 2024 • edited

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

kostasrim commented Feb 22, 2024

Choose a reason for hiding this comment

kostasrim commented Feb 21, 2024 •

edited

kostasrim Feb 22, 2024 •

edited

kostasrim Feb 22, 2024 •

edited