Track full sync completion time - #4197
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe server records replica full-sync duration, clears timing state after completion or abort, exposes the latest duration through Full sync duration reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant syncWithPrimary
participant replicaAfterLoadPrimaryRDB
participant INFO
syncWithPrimary->>syncWithPrimary: Accept +CONTINUE and start timer
syncWithPrimary->>replicaAfterLoadPrimaryRDB: Load primary RDB
replicaAfterLoadPrimaryRDB->>replicaAfterLoadPrimaryRDB: Update and clear duration
INFO->>INFO: Report primary_last_full_sync_duration_ms
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| if (server.repl_backlog == NULL) createReplicationBacklog(); | ||
| serverLog(LL_NOTICE, "PRIMARY <-> REPLICA sync: Finished with success"); | ||
| if (server.repl_full_sync_start_time) { | ||
| server.repl_full_sync_duration_ms = elapsedMs(server.repl_full_sync_start_time); |
There was a problem hiding this comment.
replicaAfterLoadPrimaryRDB() is not the completion point for dual-channel syncs. When this runs on server.repl_rdb_transfer_s, it only hands off to dualChannelSyncHandleRdbLoadCompletion() (src/replication.c:2413-2415); that helper can return after just marking REPL_DUAL_CHANNEL_RDB_LOADED if the main PSYNC is not ready yet (src/replication.c:3504-3509), and even the success path still has to drain pending_repl_data before the replica reaches steady state (src/replication.c:3456-3476). Resetting the timer here undercounts master_last_full_sync_duration_ms for dual-channel replicas; update it where dualChannelSyncSuccess() actually finishes.
There was a problem hiding this comment.
We should ensure both the single and dual channel end at comparable points, or it will look like dual channel is slower when it isn't.
I think this means that dual channel replication should mark full sync "done" before we drain the accumulated buffer. Since on single channel replication, the draining of the primary-side replication backlog would not be included in the metric.
There was a problem hiding this comment.
thats a good suggestion, thank you.
I was interested in marking full sync "done" after we drain the accumulated buffer, but there is no good way to differentiate between the backlog draining phase and the steady-state (real-time) replication stream for single-channel replication.
So in favor of simplicity and reduced complexity, excluding the backlog draining time from full sync duration time. wdyt?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4197 +/- ##
============================================
+ Coverage 76.81% 76.82% +0.01%
============================================
Files 162 162
Lines 81455 81466 +11
============================================
+ Hits 62570 62589 +19
+ Misses 18885 18877 -8
🚀 New features to boost your workflow:
|
|
Hey @murphyjacob4 @sumitk163 , please take a look at this, when you get a chance , thanks! |
| if (server.repl_backlog == NULL) createReplicationBacklog(); | ||
| serverLog(LL_NOTICE, "PRIMARY <-> REPLICA sync: Finished with success"); | ||
| if (server.repl_full_sync_start_time) { | ||
| server.repl_full_sync_duration_ms = elapsedMs(server.repl_full_sync_start_time); |
There was a problem hiding this comment.
We should ensure both the single and dual channel end at comparable points, or it will look like dual channel is slower when it isn't.
I think this means that dual channel replication should mark full sync "done" before we drain the accumulated buffer. Since on single channel replication, the draining of the primary-side replication backlog would not be included in the metric.
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
3c15b25 to
c3ec422
Compare
murphyjacob4
added
major-decision-pending
|
New metric is API surface area, so we should get a major decision. @valkey-io/core-team please vote on whether to add the new metric |
enjoy-binbin
left a comment
enjoy-binbin
left a comment
There was a problem hiding this comment.
New info fields LGTM, did not review the code yet.
From a quick look, this would be the first INFO field to use the primary terminology. The prefix also gives the impression that the duration is measured on the primary, whereas it is measured locally on the replica. Furthermore, with chained replication in standalone mode, the upstream sync source may itself be a replica. Can we simply call it |
|
Overall LGTM, though I haven't reviewed the code in detail. Just to confirm — this is an observability metric from the replica's perspective, covering the entire duration from receiving the |
Yes, except post RDB loading delta streaming time. |
madolson
added
the
needs-doc-pr
|
Ran suggested |
github-actions
Bot
added
major-decision-approved
7 participants
The current approach of manually inspecting logs to determine how long a full sync took is highly cumbersome, hence introducing a new metric called
primary_last_full_sync_duration_ms.The new metric
primary_last_full_sync_duration_ms, which measures the total time (in milliseconds) taken to complete the last full synchronization, will be reported by replicas in theINFO replicationoutput.