-
Notifications
You must be signed in to change notification settings - Fork 998
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.
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
Reduce scope of HELLO implementation #129
Conversation
Only accept protover=2 as a valid argument as that is the only thing that is supported at the moment. For all other arguments degrade to 'unknown command' The previous implementation creates issues for clients expecting the presence of the HELLO command to also signal the presence of RESP3 (as technically the command appeared in redis at the same time as support for RESP3). It also did not raise any errors when unsupported (or invalid) arguments were passed to the command (such as AUTH, SETNAME)
Perfect, thank you! |
@alisaifee can you pls double check that dragonfly_test passed - it seems that something is wrong with the error assertions. |
Ouch sorry about that - the assertion was correct, the code wasn't (messed that up while moving some code around) |
Nice, TIL. (technically, yesterday I learnt: google test.. which is awesome, so thank you for indirectly introducing me to this wondering framework). Interestingly having the full verbose assertion helped catch |
Np :) And TIL what TIL is 😄 |
There's some interesting karma at play here. I am familiar with testing on the toilet and https://testing.googleblog.com (it's nice to see that it's still up and running) and in fact I last interacted with it in 2007: https://testing.googleblog.com/2007/03/2nd-annual-google-test-automation.html. If you search hard enough there might still be some evidence of how I was connected to that 👴 |
Description
Only accept
protover=2
as a valid argument as that is the only thing that is supported at the moment. For all other arguments degrade to 'unknown command'The previous implementation creates issues for clients expecting the presence of the HELLO command to also signal
the presence of RESP3 (as technically the command appeared in redis at the same time as support for RESP3).
It also did not raise any errors when unsupported (or invalid) arguments were passed to the command (such as
AUTH
,SETNAME
)Related Issues
#126
Follow ups
SETNAME
supportAUTH
support