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

Incompatibility between DragonflyDB and redis/rueidis client #2454

Closed
thisismz opened this issue Jan 22, 2024 · 12 comments · Fixed by #2724
Closed

Incompatibility between DragonflyDB and redis/rueidis client #2454

thisismz opened this issue Jan 22, 2024 · 12 comments · Fixed by #2724
Assignees
Labels
bug Something isn't working

Comments

@thisismz
Copy link

RESP3 support missing for rueidis DragonflyDB connection?

Error when connecting to DragonflyDB with rueidis client

When trying to connect to a DragonflyDB instance using the rueidis Redis client library in Go, I get the following panic:

panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3 [recovered] panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3

This happens with the following code:

package main

import (
	"context"
	"fmt"

	"github.com/redis/rueidis"
)

func main() {
	client, err := rueidis.NewClient(rueidis.ClientOption{InitAddress: []string{"127.0.0.1:6379"}})
	if err != nil {
		panic(err)
	}
	defer client.Close()

	ctx := context.Background()
	// SET key val NX
	err = client.Do(ctx, client.B().Set().Key("key").Value("val").Nx().Build()).Error()
	if err != nil {
		panic(err)
	}
	res, err := client.Do(ctx, client.B().Get().Key("key").Build()).ToString()
	if err != nil {
		panic(err)
	}
	fmt.Println(res)
}

It seems there may be an incompatibility between DragonflyDB and the rueidis client assumptions. Specifically the error suggests rueidis expects disabled client caching or RESP3 support.

I checked the DragonflyDB docs but didn't see mention of RESP3 support or client caching handling.

Could this be added to enable better compatibility with Redis clients like rueidis? Or is there another recommended approach for connecting to DragonflyDB from Go code?

Let me know if any other details would be helpful in resolving this. Thanks!

Feel free to modify or improve this as needed. Hopefully it clearly explains the issue you are seeing. Let me know if you have any other questions!

@thisismz thisismz added the bug Something isn't working label Jan 22, 2024
@kostasrim
Copy link
Contributor

Hi @thisismz, this should be a bug (we support both RESP3 and client side tracking).

Let me get back to you on this one

@kostasrim
Copy link
Contributor

Hi @thisismz , I run your example with go 1.21.6. First, I am not quite sure how you are getting this output:

panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3 [recovered] panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3

When I run it, I just get: panic: syntax error.

How I reproduced: go mod init test && go get github.com/redis/rueidis && go run . . The good thing is at least I managed to reproduce and figure out what the issue is. We recently added support for the CLIENT TRACKING subcommand in #2139
However, it appears that we only added partial support. The only tthing we accept is CLIENT TRACKING ON/OFF. However, rueidis sends CLIENT TRACKING ON OPTIN and dragonfly fails to parse the OPTIN arguments and returns with a syntax error.

Therefore to resolve this we need to parse the OPTIN argument + test with ruedis.

@romange what's the priority of this? Also, I noticed that we need to update the docs https://www.dragonflydb.io/docs/command-reference/compatibility to include CLIENT TRACKING I am mentioning it here so we don't forget

@thisismz
Copy link
Author

Yes, it seems that you are correct; the error appears to be different between Linux and Windows. I executed this on my MacBook and received the same error as you. let me the opportunity to double-check.

However, it seems that by adding the following parameter, it appears that the issue has been resolved:

DisableCache: true
client, err := rueidis.NewClient(
	rueidis.ClientOption{
	InitAddress: []string{"127.0.0.1:6379"},
	DisableCache: true,
})

But when we set a key twice, we receive the following error:

panic: redis nil message
goroutine 1 [running]:
main.main()
        /Users/mozaffari/Documents/tst/dragonfly/main.go:22 +0x590

It seems like we might not have overwriting capabilities.

Hi @thisismz , I run your example with go 1.21.6. First, I am not quite sure how you are getting this output:

panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3 [recovered] panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3

When I run it, I just get: panic: syntax error.

How I reproduced: go mod init test && go get github.com/redis/rueidis && go run . . The good thing is at least I managed to reproduce and figure out what the issue is. We recently added support for the CLIENT TRACKING subcommand in #2139 However, it appears that we only added partial support. The only tthing we accept is CLIENT TRACKING ON/OFF. However, rueidis sends CLIENT TRACKING ON OPTIN and dragonfly fails to parse the OPTIN arguments and returns with a syntax error.

Therefore to resolve this we need to parse the OPTIN argument + test with ruedis.

@romange what's the priority of this? Also, I noticed that we need to update the docs https://www.dragonflydb.io/docs/command-reference/compatibility to include CLIENT TRACKING I am mentioning it here so we don't forget

@thisismz thisismz reopened this Jan 27, 2024
@thisismz
Copy link
Author

Hello @kostasrim

I checked again on Windows and got the same error that was reported.

panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3 [recovered] panic: Unknown subcommand or wrong number of arguments for 'TRACKING'. Try CLIENT HELP.: ClientOption.DisableCache must be true for redis not supporting client-side caching or not supporting RESP3

I'm using docker version 4.27.1 and
dragonfly v1.13.0-f39eac5bcaf7c8ffe5c433a0e8e15747391199d9
build time: 2023-12-04 15:59:48
The only way it works for me is when DisableCache: true,

@kostasrim
Copy link
Contributor

kostasrim commented Feb 10, 2024

Hi @thisismz from a quick look the flag DisableCache seems to not use the CLIENT TRACKING command at all and that's why it works. Moreover, the reason you are getting nil message is because:

err = client.Do(ctx, client.B().Set().Key("key").Value("val").Nx().Build()).Error()

You call Nx() which only sets the key if it does not already exist. Therefore if you use the same code above twice, the second time will get you a nil. You can remove Nx and the example will work fine.

It seems like we might not have overwriting capabilities

You used Nx()

@rueian
Copy link
Contributor

rueian commented Mar 13, 2024

Hi all,

rueidis can send CLIENT TRACKING ON without OPTIN by doing this:

package main

import (
	"context"
	"fmt"
	"time"

	"github.com/redis/rueidis"
)

func main() {
	client, err := rueidis.NewClient(rueidis.ClientOption{
		InitAddress:           []string{"127.0.0.1:6379"},
		ClientTrackingOptions: []string{},
	})
	if err != nil {
		panic(err)
	}
	defer client.Close()
	fmt.Println(client.DoCache(context.Background(), client.B().Get().Key("mykey").Cache(), time.Second).Error())
}

However, I found dragonfly would crash on the above client.DoCache(...) call like this:

I20240313 12:17:42.688308     9 listener_interface.cc:101] sock[7] AcceptServer - listening on port 6379
*** SIGSEGV received at time=1710332266 on cpu 0 ***
PC: @     0xaaaae1226770  (unknown)  dfly::Transaction::Refurbish()
    @     0xaaaae187cc7c        480  absl::lts_20230802::AbslFailureSignalHandler()
    @     0xffffacb3b7a0       4960  (unknown)
    @     0xaaaae12ce164        400  facade::Connection::DispatchOperations::operator()()
E20240313 12:17:46.477110     9 server_family.cc:1532] Subcommand CACHING not supported
    @     0xaaaae12d5164         32  facade::Connection::DispatchFiber()
    @     0xaaaae12d576c        384  boost::context::detail::fiber_entry<>()

I used the dragonfly docker image df-v1.15.1-d6703460242ab8aa415a93f32677c5f23b5e6ec8.

@kostasrim
Copy link
Contributor

Hi @rueian, I think we do not support CACHING subcommand and I think the issue comes from client.B().Get().Key("mykey").Cache(), specifically the Cache part

@rueian
Copy link
Contributor

rueian commented Mar 13, 2024

Hi @kostasrim, Thank you for the prompt reply!

I understand that Dragonfly doesn't support the CACHING subcommand at this moment. However, I think a reasonable reaction to that should be returning an error like -ERR Unknown subcommand or wrong number of arguments for 'CACHING'. Try CLIENT HELP. instead of crashing.

@kostasrim
Copy link
Contributor

Hi @rueian, yes I agree we should not crash, I was commenting about why it would not currently work. Thank you for reporting this, I will take care of this.

@kostasrim
Copy link
Contributor

Hi @rueian, turns out there was bigger issue on our side. I fixed it and you should be now getting a proper error message

@rueian
Copy link
Contributor

rueian commented Mar 14, 2024

Wow, thanks! That's amazing. I can't believe you fix this so fast.

@joeky888
Copy link

I think this issue should open until CLIENT TRACKING is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants