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.

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

Merged
merged 5 commits into from
Jun 9, 2022

Conversation

alisaifee
Copy link
Contributor

@alisaifee alisaifee commented Jun 9, 2022

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 support
  • AUTH support

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)
@romange
Copy link
Collaborator

romange commented Jun 9, 2022

Perfect, thank you!

@romange
Copy link
Collaborator

romange commented Jun 9, 2022

@alisaifee can you pls double check that dragonfly_test passed - it seems that something is wrong with the error assertions.
btw, you can provide a substring and not the whole string to test against.

@alisaifee
Copy link
Contributor Author

@alisaifee can you pls double check that dragonfly_test passed - it seems that something is wrong with the error assertions. btw, you can provide a substring and not the whole string to test against.

Ouch sorry about that - the assertion was correct, the code wasn't (messed that up while moving some code around)

@alisaifee
Copy link
Contributor Author

btw, you can provide a substring and not the whole string to test against.

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 s/Unknown/unknown which as it turns out would be important for lettuce as it is using a case sensitive comparison (reference)

@romange
Copy link
Collaborator

romange commented Jun 9, 2022

Np :) And TIL what TIL is 😄
Google test is accompanying me for 16 years. In Google, we have been learning about how to use it efficiently, out of all places in the toilet: https://testing.googleblog.com/2007/01/introducing-testing-on-toilet.html

@romange romange merged commit e82a378 into dragonflydb:main Jun 9, 2022
@alisaifee
Copy link
Contributor Author

Np :) And TIL what TIL is 😄 Google test is accompanying me for 16 years. In Google, we have been learning about how to use it efficiently, out of all places in the toilet: https://testing.googleblog.com/2007/01/introducing-testing-on-toilet.html

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 👴

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants