Skip to content
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

Split instantaneous_repl_total_kbps #10810

Merged

Conversation

DarrenJiang13
Copy link
Contributor

A supplement to #10062
Split instantaneous_repl_total_kbps to instantaneous_input_repl_kbps and instantaneous_output_repl_kbps.

Work:

This PR:

  • delete 1 info field:
    • instantaneous_repl_total_kbps
  • add 2 info fields:
    • instantaneous_input_repl_kbps / instantaneous_output_repl_kbps

Result:

  • master
total_net_input_bytes:26633673
total_net_output_bytes:21716596
total_net_repl_input_bytes:0
total_net_repl_output_bytes:18433052
instantaneous_input_kbps:0.02
instantaneous_output_kbps:0.00
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00
  • slave
total_net_input_bytes:18433212
total_net_output_bytes:94790
total_net_repl_input_bytes:18433052
total_net_repl_output_bytes:0
instantaneous_input_kbps:0.00
instantaneous_output_kbps:0.05
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00

@DarrenJiang13
Copy link
Contributor Author

DarrenJiang13 commented Jun 2, 2022

@soloestoy @oranagra
could you please check this PR and I will update redis-doc after this being merged.

@madolson
Copy link
Contributor

madolson commented Jun 3, 2022

Strongly against deleting the existing field. We have ops tools that rely on printing the instantaneous metric, I am sure many others do as well. I have no real objection to adding 2 more though.

@soloestoy
Copy link
Collaborator

Strongly against deleting the existing field. We have ops tools that rely on printing the instantaneous metric, I am sure many others do as well. I have no real objection to adding 2 more though.

@madolson I can't understand your word, the instantaneous_repl_total_kbps have not been released, why you rely on an unreleased metric? it doesn't make sense.

@madolson
Copy link
Contributor

madolson commented Jun 3, 2022

@soloestoy You're right, I copied the wrong variable name.

We expect only one of these numbers to be non-zero for the most part right?

@soloestoy
Copy link
Collaborator

We expect only one of these numbers to be non-zero for the most part right?

You mean instantaneous_input_repl_kbps and instantaneous_output_repl_kbps?

In fact we build the replication chain master->replica->replica->... for users, to avoid heavy replication output flow in master, so the mid replicas' input and output replication flow are non-zero for the most part.

@madolson
Copy link
Contributor

madolson commented Jun 3, 2022

I forgot about chain replicas. I suppose if we are going to have repl_total_kpbs, I have no objection to splitting it between the two.

Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

I'm OK with the change.

@oranagra oranagra moved this from In Review to Awaits merge in 7.0 Jun 4, 2022
@oranagra oranagra merged commit f558583 into redis:unstable Jun 6, 2022
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Jun 6, 2022
@oranagra oranagra moved this from Awaits merge to Unreleased in 7.0 Jun 6, 2022
@oranagra oranagra mentioned this pull request Jun 8, 2022
@oranagra oranagra moved this from Unreleased to Done in 7.0 Jun 13, 2022
Mixficsol pushed a commit to Mixficsol/redis that referenced this pull request Apr 12, 2023
…and instantaneous_output_repl_kbps. (redis#10810)

A supplement to redis#10062
Split `instantaneous_repl_total_kbps` to `instantaneous_input_repl_kbps` and `instantaneous_output_repl_kbps`. 
## Work:
This PR:
- delete 1 info field:
    - `instantaneous_repl_total_kbps`
- add 2 info fields:
    - `instantaneous_input_repl_kbps / instantaneous_output_repl_kbps`
## Result:
- master
```
total_net_input_bytes:26633673
total_net_output_bytes:21716596
total_net_repl_input_bytes:0
total_net_repl_output_bytes:18433052
instantaneous_input_kbps:0.02
instantaneous_output_kbps:0.00
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00
```
- slave
```
total_net_input_bytes:18433212
total_net_output_bytes:94790
total_net_repl_input_bytes:18433052
total_net_repl_output_bytes:0
instantaneous_input_kbps:0.00
instantaneous_output_kbps:0.05
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00
```
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…and instantaneous_output_repl_kbps. (redis#10810)

A supplement to redis#10062
Split `instantaneous_repl_total_kbps` to `instantaneous_input_repl_kbps` and `instantaneous_output_repl_kbps`. 
## Work:
This PR:
- delete 1 info field:
    - `instantaneous_repl_total_kbps`
- add 2 info fields:
    - `instantaneous_input_repl_kbps / instantaneous_output_repl_kbps`
## Result:
- master
```
total_net_input_bytes:26633673
total_net_output_bytes:21716596
total_net_repl_input_bytes:0
total_net_repl_output_bytes:18433052
instantaneous_input_kbps:0.02
instantaneous_output_kbps:0.00
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00
```
- slave
```
total_net_input_bytes:18433212
total_net_output_bytes:94790
total_net_repl_input_bytes:18433052
total_net_repl_output_bytes:0
instantaneous_input_kbps:0.00
instantaneous_output_kbps:0.05
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes
Projects
Archived in project
7.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants