Skip to content

Fix accounting for dual channel RDB bytes in replication stats #2602

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 16, 2025
Merged

Fix accounting for dual channel RDB bytes in replication stats #2602

merged 3 commits into from
Sep 16, 2025

Conversation

@sarthakaggarwal97
Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 commented Sep 11, 2025

Resolves #2545

Followed the steps to reproduce the issue, and was able to get non-zero total_net_repl_output_bytes.

(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the fix-bug-2545 branch 2 times, most recently from 5f801e9 to 456f8e9 Compare September 11, 2025 13:55
@codecov
Copy link

codecov bot commented Sep 11, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.10%. Comparing base (a47e8fa) to head (d7db493).
⚠️ Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #2602      +/-   ##
============================================
- Coverage     72.24%   72.10%   -0.15%     
============================================
  Files           127      127              
  Lines         70820    70826       +6     
============================================
- Hits          51167    51070      -97     
- Misses        19653    19756     +103
Files with missing lines Coverage Δ
src/childinfo.c 97.50% <100.00%> (+1.44%) ⬆️
src/rdb.c 76.70% <100.00%> (-0.47%) ⬇️
src/server.c 88.41% <100.00%> (-0.03%) ⬇️
src/server.h 100.00% <ø> (ø)
src/module.c 9.79% <0.00%> (ø)

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hpatro hpatro requested a review from naglera September 11, 2025 18:14
murphyjacob4
murphyjacob4 reviewed Sep 11, 2025
View reviewed changes
Copy link
Contributor

@murphyjacob4 murphyjacob4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up! Since it is a bug fix - we should mark it as needing backport.

Can we also clarify the exact behavior change in the title, so it will be clear in the changelog? Something like:

Fix accounting for dual channel RDB bytes in replication stats

@sarthakaggarwal97 sarthakaggarwal97 changed the title Fixing dual channel RDB bytes stats Fix accounting for dual channel RDB bytes in replication stats Sep 11, 2025
@sarthakaggarwal97
Copy link
Contributor Author

sarthakaggarwal97 commented Sep 11, 2025

we should mark it as needing backport

@murphyjacob4 how do we mark this? I don't see a backport label that I can add somehow.

@Nikhil-Manglore
Copy link
Member

Nikhil-Manglore commented Sep 11, 2025 •
edited
Loading

we should mark it as needing backport

@murphyjacob4 how do we mark this? I don't see a backport label that I can add somehow.

Under the projects section I think you can select the versions we want to backport it to. Then in that project you can move it to the "to be backported" section.

@ranshid ranshid moved this to To be backported in Valkey 8.0 Sep 12, 2025
@ranshid ranshid moved this to To be backported in Valkey 8.1 Sep 12, 2025
@ranshid ranshid moved this to In Progress in Valkey 9.0 Sep 12, 2025
ranshid
ranshid approved these changes Sep 15, 2025
View reviewed changes
Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sarthakaggarwal97
Copy link
Contributor Author

sarthakaggarwal97 commented Sep 15, 2025

Thanks @ranshid for reviewing!
@murphyjacob4 @madolson @naglera please take a look once you guys have some time.

murphyjacob4
murphyjacob4 approved these changes Sep 15, 2025
View reviewed changes
Copy link
Contributor

@murphyjacob4 murphyjacob4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one small comment

@murphyjacob4 murphyjacob4 merged commit 93d7cca into valkey-io:unstable Sep 16, 2025
52 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Valkey 9.0 Sep 16, 2025
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Sep 16, 2025
@sarthakaggarwal97
Fix accounting for dual channel RDB bytes in replication stats (valke…
3a60ad2 
…y-io#2602)

Resolves valkey-io#2545

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Sep 16, 2025
@sarthakaggarwal97
Fix accounting for dual channel RDB bytes in replication stats (valke…
88320cb 
…y-io#2602)

Resolves valkey-io#2545

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
@sarthakaggarwal97 sarthakaggarwal97 mentioned this pull request Sep 16, 2025
Merged
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Sep 16, 2025
@sarthakaggarwal97
Fix accounting for dual channel RDB bytes in replication stats (valke…
4a58c9c 
…y-io#2602)

Resolves valkey-io#2545

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Sep 16, 2025
@sarthakaggarwal97
Fix accounting for dual channel RDB bytes in replication stats (valke…
12eff67 
…y-io#2602)

Resolves valkey-io#2545

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Sep 16, 2025
@sarthakaggarwal97
Fix accounting for dual channel RDB bytes in replication stats (valke…
4a90f33 
…y-io#2602)

Resolves valkey-io#2545

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
rjd15372 pushed a commit to rjd15372/valkey that referenced this pull request Sep 19, 2025
@sarthakaggarwal97 @rjd15372
Fix accounting for dual channel RDB bytes in replication stats (valke…
a8a4cd6 
…y-io#2602)

Resolves valkey-io#2545 

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
zuiderkwast added a commit that referenced this pull request Sep 23, 2025
@sarthakaggarwal97 @zuiderkwast
Fix accounting for dual channel RDB bytes in replication stats (#2602)…
4779852 
… (backport in #2614)

Backport of #2602

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
zuiderkwast added a commit that referenced this pull request Sep 23, 2025
@sarthakaggarwal97 @zuiderkwast
Fix accounting for dual channel RDB bytes in replication stats (#2602)…
4d7d157 
… (backport in #2616)

Backport of #2602. Resolves #2545.

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
rjd15372 pushed a commit that referenced this pull request Sep 23, 2025
@sarthakaggarwal97 @rjd15372
Fix accounting for dual channel RDB bytes in replication stats (#2602)
7471252 
Resolves #2545 

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
@ranshid ranshid moved this from To be backported to Done in Valkey 8.1 Sep 30, 2025
@zuiderkwast
Copy link
Contributor

zuiderkwast commented Sep 30, 2025 •
edited
Loading

I'm preparing to release 8.0.6, but I can't cherry-pick this one to the 8.0 branch. There is a nasty merge conflict because of the refactoring in #2533. I'm going to skip this backport for now.

If we need it in 8.0, can you (@sarthakaggarwal97 or @enjoy-binbin) make a backporting PR for this one?

This one was already backported! It was done in #2616.

It was still in the To be backported column in the project board. That's was my confusion.

@zuiderkwast zuiderkwast moved this from To be backported to 8.0.6 (to be released) in Valkey 8.0 Sep 30, 2025
hpatro pushed a commit to hpatro/valkey that referenced this pull request Oct 3, 2025
@sarthakaggarwal97 @hpatro
Fix accounting for dual channel RDB bytes in replication stats (valke…
1a7f8b7 
…y-io#2602)

Resolves valkey-io#2545

Followed the steps to reproduce the issue, and was able to get non-zero
`total_net_repl_output_bytes`.

```
(base) ~/workspace/valkey git:[fix-bug-2545]
src/valkey-cli INFO | grep total_net_repl_output_bytes
total_net_repl_output_bytes:1788
```

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murphyjacob4 murphyjacob4 murphyjacob4 approved these changes

@ranshid ranshid ranshid approved these changes

@madolson madolson Awaiting requested review from madolson

@naglera naglera Awaiting requested review from naglera

Assignees

@sarthakaggarwal97 sarthakaggarwal97

Labels

None yet

Projects

Valkey 8.0
Status: 8.0.6
Valkey 8.1
Status: Done
Valkey 9.0
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Dual channel RDB bytes not included in stats when diskless sync is used

5 participants

@sarthakaggarwal97 @Nikhil-Manglore @zuiderkwast @murphyjacob4 @ranshid