Support for reading from replicas in valkey-benchmark #1392

bluayer · 2024-12-05T08:53:40Z

Background
When conducting performance tests using valkey-benchmark, reading from replicas was not supported. Consequently, even in cluster mode, all reads were directed to the primary nodes. This limitation made it challenging to obtain accurate metrics during workload stress testing for performance measurement or before a version upgrade.

Related issue : #900

Changes

Replaced the use of CLUSTER NODES with CLUSTER SLOTS when fetching cluster configuration. This allows for easier identification of replica slots.
Support for reading from replicas by executing the client in READONLY mode.
Support reading from replicas even during slot migrations.
Introduced a CLI option --rfr to enable reading from replicas only or all cluster nodes. A warning added to indicate that write requests might not be handled correctly when using this option.

codecov · 2024-12-05T19:52:18Z

Codecov Report

Attention: Patch coverage is 2.80374% with 104 lines in your changes missing coverage. Please review.

Project coverage is 70.82%. Comparing base (105509c) to head (a739454).
Report is 103 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/valkey-benchmark.c	2.80%	104 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1392      +/-   ##
============================================
- Coverage     70.83%   70.82%   -0.01%     
============================================
  Files           118      119       +1     
  Lines         63549    64689    +1140     
============================================
+ Hits          45013    45818     +805     
- Misses        18536    18871     +335

Files with missing lines	Coverage Δ
src/valkey-benchmark.c	`60.31% <2.80%> (+2.36%)`	⬆️

... and 63 files with indirect coverage changes

bluayer · 2024-12-08T14:39:34Z

I fully understand that the maintainers of this project are extremely busy and prioritize tasks based on the importance. However, I believe this PR could be a good contribution to this project. If you don't mind, can I ask for a PR review or a reviewer assignment? @madolson

madolson · 2024-12-10T01:55:09Z

@ranshid Can you take a look?

ranshid

Mostly LGTM. some comments. specifically I did not understand why we are not supporting reading form the primnaries as well

src/valkey-benchmark.c

1. The CLUSTER NODES command does not provide replicas' slot range. Therefore, I have changed to use the CLUSTER SLOTS command to implement read from the Replica. 2. If you use the CLUSTER NODES command, the string parsing is so long and complex that it makes the code difficult to read. In comparison, CLUSTER SLOTS makes the code much simpler and more concise. 3. As noted in the TODO comment, the migrating and importing information is not used. Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

1. Add CLI option and description for reading from replicas with cluster option. 2. If the user uses the replicas option, the READONLY command is automatically used when the client is created. Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

1. Update the slot by distinguishing whether it is a primary or a replica. 2. With the above change, the variable name "primaries" has been changed to "nodes". Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

Added a warning about writing requests with replicas option Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

No longer managing attributes about slots migration at the cluster node struct, so remove it. Signed-off-by: bluayer <[email protected]>

The `CLUSTER SLOTS` command returns a node that is responsible for multiple slot ranges multiple times. Therefore, I modify it for creating a node only once using dict, even if there are multiple slot ranges. Signed-off-by: bluayer <[email protected]>

- When users enable rfa(read_from_all) option, they can read from replica and primary. - When users enable rfro(read_from_replicas only) option, they can read from replicas only. - If users don't use any option related to replicas, they can read from primaries only. - Add READONLY when enabling option for reading all nodes Signed-off-by: bluayer <[email protected]>

bluayer · 2024-12-11T17:27:00Z

@ranshid Thanks for your review and suggestion. I fixed the lines based on your review and suggestion. Please take a look the changes.

ranshid · 2024-12-11T18:09:03Z

@ranshid Thanks for your review and suggestion. I fixed the lines based on your review and suggestion. Please take a look the changes.

Thank you @bluayer I will try to take a look today but might not get to it and I will be on vacation till Monday. So sorry in advance if this will delay to next week.

bluayer · 2024-12-12T01:00:51Z

Thank you @bluayer I will try to take a look today but might not get to it and I will be on vacation till Monday. So sorry in advance if this will delay to next week.

Thank you @ranshid for telling me that. Of course, I completely understand you because it's the holiday season! Don't worry about being postponed and have a great vacation ☺️

src/valkey-benchmark.c

Refactoring with enum, readFromReplicas, and combining two options(rfa, rfro) int single option(rfr) Signed-off-by: bluayer <[email protected]>

src/valkey-benchmark.c

ranshid

Just started to play with the benchmark before approving and I noticed some issues

src/valkey-benchmark.c

1. Modify mode option, "replicas" and "all" only 2. Allocate replicate name in replica node and determine whether it is a primary or a replica based on the presence of "replicate" 3. Adjust position of continue statement for --rfr only case 4. Change the i plus timing at option Signed-off-by: bluayer <[email protected]>

Signed-off-by: bluayer <[email protected]>

ranshid

some minor suggestions

src/valkey-benchmark.c

Signed-off-by: bluayer <[email protected]>

@ranshid

Based on @ranshid suggestion Co-authored-by: ranshid <[email protected]> Signed-off-by: Jungwoo Song <[email protected]>

Co-authored-by: ranshid <[email protected]> Signed-off-by: Jungwoo Song <[email protected]>

src/valkey-benchmark.c

Co-authored-by: ranshid <[email protected]> Signed-off-by: Jungwoo Song <[email protected]>

ranshid

LGTM

Signed-off-by: bluayer <[email protected]>

ranshid · 2024-12-19T16:32:27Z

@bluayer all looks good IMO. Thank you for taking the time to introduce and refactor!

bluayer · 2025-01-10T07:49:43Z

Hello, @madolson !
My customers are asking for this feature when they test their workload and consider switching from Redis to Valkey in these days.
Could you please discuss including the feature in the next minor version?

**Background** When conducting performance tests using `valkey-benchmark`, reading from replicas was not supported. Consequently, even in cluster mode, all reads were directed to the primary nodes. This limitation made it challenging to obtain accurate metrics during workload stress testing for performance measurement or before a version upgrade. Related issue : valkey-io#900 **Changes** 1. Replaced the use of `CLUSTER NODES` with `CLUSTER SLOTS` when fetching cluster configuration. This allows for easier identification of replica slots. 2. Support for reading from replicas by executing the client in `READONLY` mode. 3. Support reading from replicas even during slot migrations. 4. Introduced two CLI options `--rfr` to enable reading from replicas only or all cluster nodes. A warning added to indicate that write requests might not be handled correctly when using this option. --------- Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]> Signed-off-by: Jungwoo Song <[email protected]> Co-authored-by: ranshid <[email protected]>

## Summary Add description about '--rfr', read from replicas ## From release notes At 8.1.0-rc1 -> valkey-io/valkey#1392 --------- Signed-off-by: Jungwoo Song <[email protected]> Co-authored-by: Ran Shidlansik <[email protected]>

bluayer force-pushed the benchmark-read-from-replica branch 2 times, most recently from e4e100b to 3882216 Compare December 5, 2024 09:31

hwware mentioned this pull request Dec 5, 2024

Add CLUSTER FLUSHSLOT command #1384

Merged

madolson requested a review from ranshid December 10, 2024 01:55

ranshid reviewed Dec 10, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

src/valkey-benchmark.c Outdated Show resolved Hide resolved

src/valkey-benchmark.c Show resolved Hide resolved

src/valkey-benchmark.c Outdated Show resolved Hide resolved

src/valkey-benchmark.c Show resolved Hide resolved

bluayer force-pushed the benchmark-read-from-replica branch 2 times, most recently from 76545cb to d1a13ed Compare December 11, 2024 14:53

bluayer and others added 8 commits December 12, 2024 02:17

Add description for CLI

fcc240f

Added a warning about writing requests with replicas option Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

Formatting according to clang-format

ebc0fc3

Signed-off-by: bluayer <[email protected]> Signed-off-by: bluayer <[email protected]>

Remove migrating, importing attributes

136ed28

No longer managing attributes about slots migration at the cluster node struct, so remove it. Signed-off-by: bluayer <[email protected]>

bluayer force-pushed the benchmark-read-from-replica branch from 9de2f81 to bd44040 Compare December 11, 2024 17:17

ranshid reviewed Dec 16, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

Combining two options into single option "--rfr"

e1e925f

Refactoring with enum, readFromReplicas, and combining two options(rfa, rfro) int single option(rfr) Signed-off-by: bluayer <[email protected]>

bluayer force-pushed the benchmark-read-from-replica branch from 916cd5a to e1e925f Compare December 17, 2024 13:17

ranshid reviewed Dec 17, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

ranshid reviewed Dec 17, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

ranshid reviewed Dec 17, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

bluayer force-pushed the benchmark-read-from-replica branch from 5340691 to 570f5f6 Compare December 17, 2024 17:43

Explicit print with Node

18a7870

Signed-off-by: bluayer <[email protected]>

ranshid reviewed Dec 17, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

src/valkey-benchmark.c Outdated Show resolved Hide resolved

src/valkey-benchmark.c Outdated Show resolved Hide resolved

bluayer and others added 3 commits December 18, 2024 03:01

Change node_prefix to node_roles

6f804b6

Signed-off-by: bluayer <[email protected]>

Change description in CLI

67ae655

Based on @ranshid suggestion Co-authored-by: ranshid <[email protected]> Signed-off-by: Jungwoo Song <[email protected]>

Modify CLI options

4262232

Co-authored-by: ranshid <[email protected]> Signed-off-by: Jungwoo Song <[email protected]>

ranshid reviewed Dec 18, 2024

View reviewed changes

src/valkey-benchmark.c Outdated Show resolved Hide resolved

Remove unnecessary printing stderr

54f8490

Co-authored-by: ranshid <[email protected]> Signed-off-by: Jungwoo Song <[email protected]>

ranshid approved these changes Dec 19, 2024

View reviewed changes

Formatting with clang

a739454

Signed-off-by: bluayer <[email protected]>

ranshid merged commit e9a1fe0 into valkey-io:unstable Dec 19, 2024
50 checks passed

ranshid added the release-notes This issue should get a line item in the release notes label Dec 19, 2024

ranshid added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Jan 14, 2025

This was referenced Mar 25, 2025

Add benchmark option '--rfr' bluayer/valkey-doc#1

Open

Add description about benchmark option '--rfr' valkey-io/valkey-doc#254

Merged

Support for reading from replicas in valkey-benchmark #1392

Support for reading from replicas in valkey-benchmark #1392

Uh oh!

Conversation

bluayer commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bluayer commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Dec 10, 2024

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluayer commented Dec 11, 2024

Uh oh!

ranshid commented Dec 11, 2024

Uh oh!

bluayer commented Dec 12, 2024

Uh oh!

Uh oh!

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

ranshid commented Dec 19, 2024

Uh oh!

Uh oh!

bluayer commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bluayer commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

bluayer commented Dec 8, 2024 •

edited

Loading