Skip to content

Added URI support to redis-benchmark (cli and benchmark share the same uri-parsing methods)#9314

Merged
oranagra merged 9 commits intoredis:unstablefrom
filipecosta90:uri.cli_common
Sep 14, 2021
Merged

Added URI support to redis-benchmark (cli and benchmark share the same uri-parsing methods)#9314
oranagra merged 9 commits intoredis:unstablefrom
filipecosta90:uri.cli_common

Conversation

@filipecosta90
Copy link
Contributor

@filipecosta90 filipecosta90 commented Aug 3, 2021 •
edited by oranagra
Loading

  • Add -u <uri> command line option to support redis:// URI scheme.
  • included server connection information object (struct cliConnInfo), used to describe an ip:port pair, db num user input, and user:pass to avoid a large number of function arguments.
  • Using sds on connection info strings for redis-benchmark/redis-cli

To test solely redis-benchmark:

tclsh tests/test_helper.tcl --single integration/redis-benchmark

revamp of #4507 ( cc @itamarhaber )

yoav-steinberg
yoav-steinberg reviewed Aug 18, 2021
View reviewed changes
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

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

More-or-less ok.
Kind of surprised by the mixup of sds/char* and not so safe/strict url parsing. But this was already in redis-cli to begin with. So your call regarding what to fix and what not...

@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Aug 25, 2021
@oranagra
Copy link
Member

oranagra commented Aug 25, 2021

generally, the changes seem ok.
@filipecosta90 are you gonna pick this up, or shall i attempt to resolve the remaining problems?

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Aug 25, 2021
@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Aug 25, 2021

generally, the changes seem ok.
@filipecosta90 are you gonna pick this up, or shall i attempt to resolve the remaining problems?

fixing all comments until EOD @oranagra :)

@filipecosta90 @yoav-steinberg
Update src/cli_common.c
f8dfcb3 
Co-authored-by: yoav-steinberg <[email protected]>
@oranagra
Copy link
Member

oranagra commented Sep 9, 2021

@filipecosta90 can you take care of the CI failure and see if you can handle the memory "leak" issues?
If complicated, I think we can drop live with them too, unless @yoav-steinberg disagrees..

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Sep 9, 2021

@filipecosta90 can you take care of the CI failure and see if you can handle the memory "leak" issues?
If complicated, I think we can drop live with them too, unless @yoav-steinberg disagrees..

doing so @oranagra 👍

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Sep 9, 2021 •
edited
Loading

@oranagra @yoav-steinberg using always sds on conn info on the benchmark/cli. Can you guys check it?
Edit: also included some extra benchmark tests to validate that indeed we're doing the proper parsing, et all...

@filipecosta90 filipecosta90 mentioned this pull request Sep 9, 2021
Closed
yoav-steinberg
yoav-steinberg reviewed Sep 11, 2021
View reviewed changes
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

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

Looks good.
Note regarding the new tests, they are piratically identical. Worth trying to unify them do reduce code duplication.
You can search for foreach usage under tests/ to see example of how we avoid code duplication when we want to run the same test with slightly different arguments.

@filipecosta90
Copy link
Contributor Author

filipecosta90 commented Sep 13, 2021

Looks good.
Note regarding the new tests, they are piratically identical. Worth trying to unify them do reduce code duplication.
You can search for foreach usage under tests/ to see example of how we avoid code duplication when we want to run the same test with slightly different arguments.

@yoav-steinberg tried to add two common code blocks across the tests to reduce duplication. can you check if it's OK now?

yoav-steinberg
yoav-steinberg approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

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

Excellent

@oranagra
Copy link
Member

oranagra commented Sep 14, 2021

triggered daily (partial) daily CI: https://github.com/redis/redis/actions/runs/1234446071

@oranagra oranagra merged commit b5a879e into redis:unstable Sep 14, 2021
@filipecosta90 filipecosta90 deleted the uri.cli_common branch September 14, 2021 17:06
@oranagra oranagra mentioned this pull request Sep 24, 2021
Merged
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

@itamarhaber itamarhaber Awaiting requested review from itamarhaber

+1 more reviewer

@yoav-steinberg yoav-steinberg yoav-steinberg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@filipecosta90 @oranagra @yoav-steinberg