Valkey updates#2
Open
dvalinrh wants to merge 2 commits into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR updates the Valkey benchmark wrapper script (valkey.sh) to increase default request volume, restructure per-iteration benchmark execution with explicit systemd lifecycle management, scope CSV extraction per iteration, add multi-iteration result aggregation with outlier trimming, and update result persistence. It also removes stale verification JSON data and replaces README.md with an expanded user guide. ChangesValkey benchmark script changes
Verification data cleanup
README documentation rewrite
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 iteration(iter, threads)
execute_valkey->>systemctl: start valkey
systemctl-->>execute_valkey: --wait, status
execute_valkey->>ValkeyBenchmark: run benchmark, write valkey_iter_<iter>_*.csv
execute_valkey->>systemctl: stop valkey
MainFlow->>generate_combine_report: aggregate all iterations
generate_combine_report-->>MainFlow: valkey_results.csv (averaged, trimmed)
MainFlow->>MainFlow: 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
Comment on lines
+250
to
+278
| generate_combine_report() | ||
| { | ||
| rm -f valkey_results.csv | ||
| for tst in $test_list | ||
| do | ||
| high=0 | ||
| low=9999999 | ||
| sum=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) | ||
| let "sum=${sum}+${rval}" | ||
| if [[ $rval -gt $high ]]; then | ||
| high=$rval | ||
| fi | ||
| if [[ $rval -lt $low ]]; then | ||
| low=$rval | ||
| fi | ||
| done | ||
| if [[ $to_times_to_run -gt 4 ]]; then | ||
| let "samples=${to_times_to_run}-2" | ||
| let "sum=${sum}-${high}-${low}" | ||
| else | ||
| samples=${to_times_to_run} | ||
| fi | ||
| avg=$(echo $sum/$samples | bc) | ||
| echo ${tst},$avg >> valkey_results.csv | ||
| done | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Aggregation is fragile for high RPS and missing samples.
Two robustness gaps in generate_combine_report:
low=9999999is a fixed sentinel. valkey-benchmark RPS for simple ops (e.g.PING,GET) can exceed ~10M on fast hardware/many threads; if every sample is above the sentinel,lownever updates and the outlier trim subtracts a bogus value, skewing the average.- If a test is missing from any
valkey_iter_${iter}.csv,rvalis empty and bothlet "sum=${sum}+${rval}"and[[ $rval -gt $high ]]raise arithmetic errors for that iteration.
Also, avg=$(echo $sum/$samples | bc) uses bc's default scale 0, so the reported average RPS is integer-truncated.
♻️ Suggested hardening
high=0
- low=9999999
+ low=
sum=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)
+ [[ -z $rval ]] && continue
let "sum=${sum}+${rval}"
if [[ $rval -gt $high ]]; then
high=$rval
fi
- if [[ $rval -lt $low ]]; then
+ if [[ -z $low || $rval -lt $low ]]; then
low=$rval
fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| generate_combine_report() | |
| { | |
| rm -f valkey_results.csv | |
| for tst in $test_list | |
| do | |
| high=0 | |
| low=9999999 | |
| sum=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) | |
| let "sum=${sum}+${rval}" | |
| if [[ $rval -gt $high ]]; then | |
| high=$rval | |
| fi | |
| if [[ $rval -lt $low ]]; then | |
| low=$rval | |
| fi | |
| done | |
| if [[ $to_times_to_run -gt 4 ]]; then | |
| let "samples=${to_times_to_run}-2" | |
| let "sum=${sum}-${high}-${low}" | |
| else | |
| samples=${to_times_to_run} | |
| fi | |
| avg=$(echo $sum/$samples | bc) | |
| echo ${tst},$avg >> valkey_results.csv | |
| done | |
| } | |
| generate_combine_report() | |
| { | |
| rm -f valkey_results.csv | |
| for tst in $test_list | |
| do | |
| high=0 | |
| low= | |
| sum=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) | |
| [[ -z $rval ]] && continue | |
| let "sum=${sum}+${rval}" | |
| if [[ $rval -gt $high ]]; then | |
| high=$rval | |
| fi | |
| if [[ -z $low || $rval -lt $low ]]; then | |
| low=$rval | |
| fi | |
| done | |
| if [[ $to_times_to_run -gt 4 ]]; then | |
| let "samples=${to_times_to_run}-2" | |
| let "sum=${sum}-${high}-${low}" | |
| else | |
| samples=${to_times_to_run} | |
| fi | |
| avg=$(echo $sum/$samples | bc) | |
| echo ${tst},$avg >> valkey_results.csv | |
| done | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 258-258: to_times_to_run is referenced but not assigned.
(SC2154)
[info] 258-258: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 260-260: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 260-260: Double quote to prevent globbing and word splitting.
(SC2086)
[style] 261-261: Instead of 'let expr', prefer (( expr )) .
(SC2219)
[style] 270-270: Instead of 'let expr', prefer (( expr )) .
(SC2219)
[style] 271-271: Instead of 'let expr', prefer (( expr )) .
(SC2219)
[info] 275-275: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 276-276: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 276-276: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 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.
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
1 participant
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