Skip to content

redis-benchmark: add tests, --version, a minor bug fixes#7947

Merged
oranagra merged 4 commits intoredis:unstablefrom
filipecosta90:redis.benchmark.tcl
Oct 26, 2020
Merged

redis-benchmark: add tests, --version, a minor bug fixes#7947
oranagra merged 4 commits intoredis:unstablefrom
filipecosta90:redis.benchmark.tcl

Conversation

@filipecosta90
Copy link
Contributor

@filipecosta90 filipecosta90 commented Oct 22, 2020

added tests

This PR is a maintenance one for redis-benchmark, adding the following tests for standalone redis:

  • full test suite
  • multi-thread full test suite
  • pipelined commands full test suite
  • arbitrary command

added --version

Apart from it, and to make it easier to reference redis-benchmark version it applies the same version format as seen on redis-cli.

$ redis-benchmark --version
redis-benchmark 255.255.255 (git:7223d1d0)

@oranagra
Copy link
Member

oranagra commented Oct 25, 2020

@filipecosta90 thank you for this.
I see the additional test takes 45 seconds, since we're not really looking doing benchmark (don't care about the performance results), i would like to find a way to trim it down to just a few seconds.

sine the test names on success / error are printed without context, i think it would be better to add a benchmark: prefix to the individual tests.

it's nice that we now have coverage to make sure redis benchmark doesn't crash, but i think we may want to do a bit more to validate that the output on success seems correct.

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Oct 25, 2020

Tks for the initial review Oran. Will proceed with the changes and report back 👍

@filipecosta90 filipecosta90 mentioned this pull request Oct 25, 2020
Closed
@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Oct 25, 2020

@oranagra I've changed the naming and added further clauses as per the above comment.
Apart from it while adding more tests and proper checks on each test I've noticed that redis-benchmark with pipeline is not enforcing upper bound to number of requests. I've opened an issue describing the problem #7960 and added the the fix and more tests to this PR.

@oranagra oranagra linked an issue Oct 25, 2020 that may be closed by this pull request
[BUG] redis-benchmark with pipeline not enforcing upper bound to number of requests. #7960
Closed
oranagra
oranagra reviewed Oct 25, 2020
View reviewed changes
oranagra
oranagra reviewed Oct 25, 2020
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

@filipecosta90 i see the test still takes some 25 seconds.
please try to shave it down further.

i see most tests only run some 1000 commands, so maybe it's the keyspace length test that's taking most time.

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Oct 25, 2020

@filipecosta90 i see the test still takes some 25 seconds.
please try to shave it down further.

i see most tests only run some 1000 commands, so maybe it's the keyspace length test that's taking most time.

Thank you for the quick review @oranagra. I've tried to nail down further the tests duration and now running solely the redis-benchmark locally it's showming 1 sec duration:

$ tclsh tests/test_helper.tcl --verbose --single integration/redis-benchmark 
Cleanup: may take some time... OK
Starting test server at port 11111
[ready]: 657206
Testing integration/redis-benchmark
[ready]: 657205
[ready]: 657207
[ready]: 657208
[ready]: 657209
[ready]: 657210
[ready]: 657211
[ready]: 657212
[ready]: 657213
[ready]: 657214
[ready]: 657215
[ready]: 657216
[ready]: 657217
[ready]: 657218
[ready]: 657219
[ready]: 657220
=== (benchmark) Starting server 127.0.0.1:21611 ok
=== () Starting server 127.0.0.1:21612 ok
[ok]: benchmark: set,get
[ok]: benchmark: full test suite
[ok]: benchmark: multi-thread set,get
[ok]: benchmark: pipelined full set,get
[ok]: benchmark: arbitrary command
[ok]: benchmark: keyspace length
[1/1 done]: integration/redis-benchmark (1 seconds)

                   The End

Execution time of different units:
  1 seconds - integration/redis-benchmark

\o/ All tests passed without errors!

Cleanup: may take some time... OK

Can you check on your side if it also takes only that?

oranagra
oranagra reviewed Oct 25, 2020
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

ohh, so it's probably just the slowness of the github actions.
up to 5 or 10 seconds on your local computers seems fine (if you feel you rather roll some of these changes back that's ok with me.)

@oranagra oranagra changed the title Kicked off integration tests for redis-benchmark. Included --version flag on redis-benchmark redis-benchmark: add tests, --version, a minor bug fixes Oct 26, 2020
@oranagra oranagra merged commit 01acfa7 into redis:unstable Oct 26, 2020
@oranagra
Copy link
Member

oranagra commented Oct 26, 2020

@filipecosta90 looks like this test it failing when the tests are executed with --tls
https://github.com/redis/redis/pull/7965/checks?check_run_id=1309191804
i guess your follow up PR solves it?

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Oct 26, 2020

@filipecosta90 looks like this test it failing when the tests are executed with --tls
https://github.com/redis/redis/pull/7965/checks?check_run_id=1309191804
i guess your follow up PR solves it?

@oranagra yes here.
Do you want to separate this fix from that PR? or no need?

@oranagra
Copy link
Member

oranagra commented Oct 27, 2020

@filipecosta90 how can that fix the failing tests? (without adding TLS to redis-benchmark, or skipping the tests in that mode).

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 4, 2020
@filipecosta90 @oranagra
redis-benchmark: add tests, --version, a minor bug fixes (redis#7947)
3db6ba1 
- add test suite coverage for redis-benchmark
- add --version (similar to what redis-cli has)
- fix bug sending more requests than intended when pipeline > 1.
- when done sending requests, avoid freeing client in the write handler, in theory before
  responses are received (probably dead code since the read handler will call clientDone first)

Co-authored-by: Oran Agra <[email protected]>
This was referenced Apr 26, 2021
Closed
Closed
@filipecosta90 filipecosta90 mentioned this pull request May 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] redis-benchmark with pipeline not enforcing upper bound to number of requests.

2 participants

@filipecosta90 @oranagra