Skip to content

benchmark getRedisConfig exit only when meet NOAUTH error#11096

Merged
soloestoy merged 1 commit intoredis:unstablefrom
soloestoy:bench-skip-config
Nov 28, 2022
Merged

benchmark getRedisConfig exit only when meet NOAUTH error#11096
soloestoy merged 1 commit intoredis:unstablefrom
soloestoy:bench-skip-config

Conversation

@soloestoy
Copy link
Contributor

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

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
Contributor Author

soloestoy commented Aug 9, 2022

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
Contributor Author

soloestoy commented Aug 9, 2022

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
Loading

@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
Loading

@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

filipecosta90 commented Aug 9, 2022

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
Contributor Author

soloestoy commented Aug 9, 2022

OK, makes sense

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

3 participants

@soloestoy @oranagra @filipecosta90