Valkey updates#2
Open
dvalinrh wants to merge 3 commits into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR updates the Valkey benchmark wrapper to increase the default request volume, manage Valkey per thread and iteration, scope result extraction, aggregate iteration results, update artifact persistence, remove stale verification data, and replace the README with a comprehensive user guide. ChangesValkey benchmark workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MainFlow
participant execute_valkey
participant systemctl
participant ValkeyBenchmark
participant generate_combine_report
MainFlow->>execute_valkey: run configured benchmark iterations
execute_valkey->>systemctl: start Valkey and wait for readiness
systemctl-->>execute_valkey: return service status
execute_valkey->>ValkeyBenchmark: run thread benchmark and write iteration CSVs
execute_valkey->>systemctl: stop Valkey
MainFlow->>generate_combine_report: aggregate iteration RPS values
generate_combine_report-->>MainFlow: write valkey_results.csv
MainFlow->>MainFlow: stop PCP and save results
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@valkey/valkey.sh`:
- Around line 250-278: The aggregation in generate_combine_report is brittle for
very high RPS and missing per-iteration rows; update the loop over test_list/seq
so it safely handles absent grep matches from valkey_iter_${iter}.csv without
arithmetic failures, and replace the fixed low sentinel with a value derived
from the first valid sample (or another unbounded initialization). Also adjust
the avg calculation at the end of generate_combine_report so it preserves
fractional precision instead of truncating to an integer.
- Around line 191-197: The Valkey startup flow in valkey.sh uses a malformed
systemctl invocation, so the benchmark may begin before the service is ready.
Fix the startup sequence around the systemctl start/status calls by replacing
the invalid wait usage with a valid readiness check or wait-capable start
pattern before running valkey-benchmark, and keep the benchmark launch dependent
on Valkey being fully up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 72a04337-916c-45ee-aea8-987350e78280
📒 Files selected for processing (3)
README.mdvalkey/valkey.shvalkey/valkey_verify.json
💤 Files with no reviewable changes (1)
- valkey/valkey_verify.json
sayalibhavsar
requested changes
Jul 9, 2026
|
|
||
| ``` | ||
| Valkey Options: | ||
| --commit <value>: Specify a git commit or tag to use for Valkey installation. |
| 19. **LRANGE_600**: List range query retrieving 600 elements. | ||
|
|
||
| 20. **MSET**: Multiple SET operations in single command. | ||
|
|
There was a problem hiding this comment.
https://gist.github.com/sayalibhavsar/5c81c12078a73de108dbc89648b3a9bf
Operations list says 20, but valkey-benchmark also runs XADD by default which isn't listed
- XADD: Stream data structure - append entry to a stream.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
valkey/valkey.sh (1)
260-280: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
samplesdenominator ignores skipped iterations, skewing the average.When a
valkey_iter_${iter}.csvrow is missing, Line 263-265continues without adding tosum, butsamplesis still fixed toto_times_to_run(orto_times_to_run-2). The average is then divided by a count larger than the number of values actually summed, understating RPS. Track the count of valid samples instead of assuming a full set.♻️ Suggested fix
high=0 low="" sum=0 + count=0 for iter in $(seq 1 1 $to_times_to_run); do file="valkey_iter_${iter}.csv" rval=$(grep ^${tst}, $file | cut -d, -f2 |cut -d. -f1) if [[ -z $rval ]]; then - echo Error in parsing alkey_iter_${iter}.csv + echo "Error in parsing valkey_iter_${iter}.csv" continue fi let "sum=${sum}+${rval}" + count=$((count+1)) if [[ $rval -gt $high ]]; then high=$rval fi if [[ $low == "" ]] || [[ $rval -lt $low ]]; then low=$rval fi done - if [[ $to_times_to_run -gt 4 ]]; then - let "samples=${to_times_to_run}-2" + if [[ $count -gt 4 ]]; then + let "samples=${count}-2" let "sum=${sum}-${high}-${low}" else - samples=${to_times_to_run} + samples=${count} fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@valkey/valkey.sh` around lines 260 - 280, Track the number of successfully parsed rows in the iteration loop, incrementing it only after a valid rval is found, and use that count to determine samples instead of to_times_to_run. Update the outlier-removal logic so high and low are excluded only when the valid sample count supports it, while preserving the existing sum handling in the loop around valkey_iter_${iter}.csv parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@valkey/valkey.sh`:
- Around line 260-280: Track the number of successfully parsed rows in the
iteration loop, incrementing it only after a valid rval is found, and use that
count to determine samples instead of to_times_to_run. Update the
outlier-removal logic so high and low are excluded only when the valid sample
count supports it, while preserving the existing sum handling in the loop around
valkey_iter_${iter}.csv parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 273622e9-7d15-41ee-a581-4971edab5eff
📒 Files selected for processing (2)
README.mdvalkey/valkey.sh
💤 Files with no reviewable changes (1)
- README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding valkey test.
This replaces the redis test in phoronix. Addinf to the zathras test defs will occur after push.
JIRA: https://redhat.atlassian.net/browse/RPOPC-1265
Git issue: #1
Run info
Use default settings, 5 iterations is enough to produce stable results.
Code has been reviewed through claude.
Results csv file
Note: Time period given is the time for 1 iteration. There is no way to differentiate the individual tests. pcp will have the time periods for the iterations, this is just to keep the results csv files all the same.
Value present in the csv file is the average for all the iterations.
Test general meta start
Test: valkey
Results version: v1.00
Host: m6i.xlarge
Sys environ: aws
Tuned: virtual-guest
OS: 6.12.0-124.38.1.el10_1.x86_64
Numa nodes: 1
CPU family: Intel(R) Xeon(R) Platinum 8375C CPU @ 2.90GHz
Number cpus: 4
Memory: 15813884kB
Test general meta end
Test,RPS,Start_Date,End_Date
XADD,159957,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
MSET_10_keys,159792,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LRANGE_600_first_600_elements,20748,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LRANGE_500_first_500_elements,23492,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LRANGE_300_first_300_elements,37181,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LRANGE_100_first_100_elements,84175,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LPUSH_needed_to_benchmark_LRANGE,199933,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
ZPOPMIN,199933,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
ZADD,199933,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
SPOP,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
HSET,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
SADD,199933,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
RPOP,199966,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LPOP,199933,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
RPUSH,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
LPUSH,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
INCR,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
GET,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
SET,199900,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
PING_MBULK,199933,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z
PING_INLINE,199966,2026-07-08T11:19:26Z,2026-07-08T11:21:04Z