Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

benchmark getRedisConfig exit only when meet NOAUTH error #11096

Merged
merged 1 commit into from Nov 28, 2022
Merged

benchmark getRedisConfig exit only when meet NOAUTH error #11096

merged 1 commit into from Nov 28, 2022

Conversation

soloestoy
Copy link
Collaborator

@soloestoy soloestoy commented Aug 9, 2022 •
edited by oranagra

redis-benchmark: when trying to get the CONFIG before benchmark,
avoid printing any warning on most errors (e.g. NOPERM error).
avoid aborting the benchmark on NOPERM.
keep the warning only when we abort the benchmark on a NOAUTH error

oranagra
oranagra reviewed Aug 9, 2022
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.

IIRC in the past it used to cause the benchmark to fail, and we changed it to just print a warning and resume.
do you wish to hide the warning?

I think i'd prefer make it implicitly ignore the warning (by default) rather than add a new command line option.

@soloestoy
Copy link
Collaborator Author

I prefer ignore the warning too, but I saw in #8869 @filipecosta90 made it as a fatal error, do we all agree ignore the warning?

@oranagra
Copy link
Member

oranagra commented Aug 9, 2022

it's fatal only in case the error was an authentication error (indicating that we are also not likely to succeed in the benchmark itself).
i'm not sure the warning is that harmful, but arguably the very fact we won't print the configuration is an indication the CONFIG GET failed, so if the warning bothers you so much, i think we can only print it in the cases were we abort the test.

@soloestoy
Copy link
Collaborator Author

to make it simple, let's ignore the error.

@soloestoy soloestoy changed the title add --skip-config option for benchmark benchmark ignore CONFIG GET error Aug 9, 2022
@oranagra
Copy link
Member

oranagra commented Aug 9, 2022

I think we do wanna use this error for an early abort.
FAFAK it's the first attempt we made to communicate to the dest, and in case we have auth issues, it'll look nicer to abort now, rather than spin up all the benchmark connection and have them fail.

i might be missing something since i don't know this code well.
@filipecosta90 please provide your feedback.

@filipecosta90
Copy link
Contributor

filipecosta90 commented Aug 9, 2022 •
edited

@soloestoy WRT

I prefer ignore the warning too, but I saw in #8869 @filipecosta90 made it as a fatal error, do we all agree ignore the warning?

As stated on PR
#8869 it fixed wrongfully failing on config fetch error (warning only). This only affects RE. ( reverts #8855 ), meaning we don't exit on config fetch error. Let me double check

@oranagra
Copy link
Member

oranagra commented Aug 9, 2022 •
edited

@filipecosta90 the two questions are:

  1. if the warning is necessary, or is it sufficient that we just don't print the configuration
  2. if it is necessary to abort on AUTH related errors (rather than proceed and fail later)

IMHO the warning is unnecessary, but to fail early is still desired.

@filipecosta90
Copy link
Contributor

IMHO the warning is unnecessary, but to fail early is still desired.

agree with you. let's discard the warning but keep the failure ASAP on AUTH

@soloestoy
Copy link
Collaborator Author

OK, makes sense

oranagra
oranagra approved these changes Nov 22, 2022
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.

sorry for the delay, must have missed your respond to my last comment.

@soloestoy soloestoy added the release-notes indication that this issue needs to be mentioned in the release notes label Nov 22, 2022
@soloestoy soloestoy merged commit f0005b5 into redis:unstable Nov 28, 2022
@soloestoy soloestoy mentioned this pull request Nov 29, 2022
Open
@oranagra oranagra changed the title benchmark ignore CONFIG GET error benchmark getRedisConfig exit only when meet NOAUTH error Dec 6, 2022
oranagra pushed a commit to oranagra/redis that referenced this pull request Dec 11, 2022
@soloestoy @oranagra
benchmark getRedisConfig exit only when meet NOAUTH error (redis#11096)
4272efe 
redis-benchmark: when trying to get the CONFIG before benchmark,
avoid printing any warning on most errors (e.g. NOPERM error).
avoid aborting the benchmark on NOPERM.
keep the warning only when we abort the benchmark on a NOAUTH error

(cherry picked from commit f0005b5)
@oranagra oranagra mentioned this pull request Dec 11, 2022
Merged
oranagra pushed a commit that referenced this pull request Dec 12, 2022
@soloestoy @oranagra
benchmark getRedisConfig exit only when meet NOAUTH error (#11096)
fba49b1 
redis-benchmark: when trying to get the CONFIG before benchmark,
avoid printing any warning on most errors (e.g. NOPERM error).
avoid aborting the benchmark on NOPERM.
keep the warning only when we abort the benchmark on a NOAUTH error

(cherry picked from commit f0005b5)
Mixficsol pushed a commit to Mixficsol/redis that referenced this pull request Apr 12, 2023
@soloestoy @Mixficsol
benchmark getRedisConfig exit only when meet NOAUTH error (redis#11096)
022eb44 
redis-benchmark: when trying to get the CONFIG before benchmark,
avoid printing any warning on most errors (e.g. NOPERM error).
avoid aborting the benchmark on NOPERM.
keep the warning only when we abort the benchmark on a NOAUTH error
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
@soloestoy @madolson
benchmark getRedisConfig exit only when meet NOAUTH error (redis#11096)
7d0abe1 
redis-benchmark: when trying to get the CONFIG before benchmark,
avoid printing any warning on most errors (e.g. NOPERM error).
avoid aborting the benchmark on NOPERM.
keep the warning only when we abort the benchmark on a NOAUTH error
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
@soloestoy @enjoy-binbin
benchmark getRedisConfig exit only when meet NOAUTH error (redis#11096)
b0b5390 
redis-benchmark: when trying to get the CONFIG before benchmark,
avoid printing any warning on most errors (e.g. NOPERM error).
avoid aborting the benchmark on NOPERM.
keep the warning only when we abort the benchmark on a NOAUTH error
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
release-notes indication that this issue needs to be mentioned in the release notes
Projects
Redis 7.0 Backport
Status: Done
Redis Triage
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@soloestoy @oranagra @filipecosta90