Fixed Cache fetch error on Redis Cluster #4056

kamijin-fanta · 2021-04-08T08:03:37Z

What this PR does:
Redis Cluster may not support MGet. In that case, fall back to multiple Get.

Which issue(s) this PR fixes:
Fixes #4053

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

stevesg

Looks good, just one question. Looks like a useful fix for people using Redis.

stevesg · 2021-04-12T13:02:34Z

pkg/chunk/cache/redis_client.go

-	for i, val := range cmd.Val() {
-		if val != nil {
-			ret[i] = StringToBytes(val.(string))
+	_, isCluster := c.rdb.(*redis.ClusterClient)


Just a question to better understand the fix (I'm not an expect on Redis or the Go bindings).

This is working around an issue/limitation in the bindings, correct? Or a limitation in a particular server version? I only ask because my first reaction to this is that checking the type of an interface feels wrong. If it's the only option, then we'll have to live with it of course.

Either way, it might be nice to put a comment here to explain why we have to check the type.

Thank you for the review!

I thought the same thing, but I think this method is the simplest. The Redis client is initialized with the NewUniversalClient .

https://github.com/cortexproject/cortex/blob/v1.8.0/pkg/chunk/cache/redis_client.go#L70

This is the implementation.

https://github.com/go-redis/redis/blob/v8.8.0/universal.go#L199-L206

UniversalClient can take Client and ClusterClient. Single node or sentinel if Client is set. mget is always supported. Redis Cluster configuration if ClusterClient is set. mget may not be supported.

The only way to check if the Universal Client is Redis Cluster is by type assertion.

Added a comment.

stevesg

LGTM

pstibrany

Thank you. I don't know much about redis client, but given the comment, it looks reasonable.

pracucci · 2021-04-16T13:11:02Z

pkg/chunk/cache/redis_client.go

+			cmd := c.rdb.Get(ctx, key)
+			if err := cmd.Err(); err != nil {
+				return nil, err
+			}
+			ret[i] = StringToBytes(cmd.Val())


If a key doesn't exist, then ret[i] is expected to be nil. I don't think it's the case here.

@pracucci thanks for reviewing! c.rdb.MGet(...).Val() returns []interface{}, but c.rdb.Get(...).Val() returns string. If the value is not found, it will return an empty string.

If the value is not found, it will return an empty string.

I think this break the behaviour (see my previous comment above). Having a unit test to prove / disprove it would be great.

understood. Thank you!

Processed the empty values and added the tests. The tests confirm that the behavior is the same for Single and Cluster configurations of Redis.

pracucci · 2021-04-26T07:52:16Z

@kamijin-fanta 👋 Would you have some time to take a look at my last comment, please? Thanks!

gouthamve · 2021-04-28T15:13:18Z

Can you please rebase against master to pull in #4137 and move the changelog entry to the top?

pracucci

Thanks a lot for addressing my feedback! The logic LGTM. I've left a couple of last comments and then we're good to go!

pracucci · 2021-04-29T15:27:06Z

CHANGELOG.md

@@ -73,6 +73,7 @@
 * [BUGFIX] Frontend, Query-scheduler: allow querier to notify about shutdown without providing any authentication. #4066
 * [BUGFIX] Querier: fixed race condition causing queries to fail right after querier startup with the "empty ring" error. #4068
 * [BUGFIX] Compactor: Increment `cortex_compactor_runs_failed_total` if compactor failed compact a single tenant. #4094
+* [BUGFIX] Fixed cache fetch error on Redis Cluster. #4056


Please rebase master and move this CHANGELOG entry to the top. We've cut 1.9 release in the meanwhile. Thanks!

@kamijin-fanta This commit is still valid. Could you address it, please?

pracucci · 2021-04-29T15:34:54Z

pkg/chunk/cache/redis_client_test.go

+
+	clients := []*RedisClient{single, cluster}
+	for i, c := range clients {
+		meg := []string{"run on single redis client", "run on cluster redis client"}[i]


It's a little convoluted. Instead of passing meg (I guess you meant msg) to each assertion function, you can run the test within a t.Run(description, func(t *testing.T) { /* your test logic here */}) and having the description only passed to t.Run().

Following this example, a common pattern is using table testing to define the test cases. As example, I'm picking a random test where we use table testing:

cortex/pkg/chunk/schema_config_test.go

Lines 306 to 312 in 5009276

func TestSchemaConfig_Validate(t *testing.T) {

t.Parallel()

tests := map[string]struct {

config *SchemaConfig

expected *SchemaConfig

err error

thanks. fixed to assigned name and client to slice.

Signed-off-by: kamijin_fanta <[email protected]>

pracucci

Thanks @kamijin-fanta! LGTM. Please address the comment in the CHANGELOG and then we can merge. Thanks again!

pracucci · 2021-05-13T13:49:18Z

CHANGELOG.md

@@ -73,6 +73,7 @@
 * [BUGFIX] Frontend, Query-scheduler: allow querier to notify about shutdown without providing any authentication. #4066
 * [BUGFIX] Querier: fixed race condition causing queries to fail right after querier startup with the "empty ring" error. #4068
 * [BUGFIX] Compactor: Increment `cortex_compactor_runs_failed_total` if compactor failed compact a single tenant. #4094
+* [BUGFIX] Fixed cache fetch error on Redis Cluster. #4056


@kamijin-fanta This commit is still valid. Could you address it, please?

pull-request-size bot added the size/S label Apr 8, 2021

kamijin-fanta force-pushed the redis-cluster branch from f755bd6 to 6c8894b Compare April 8, 2021 08:04

stevesg reviewed Apr 12, 2021

View reviewed changes

kamijin-fanta force-pushed the redis-cluster branch from 6c8894b to 612b98d Compare April 13, 2021 03:24

pull-request-size bot added size/M and removed size/S labels Apr 13, 2021

kamijin-fanta force-pushed the redis-cluster branch from 612b98d to 2915141 Compare April 13, 2021 04:34

stevesg approved these changes Apr 13, 2021

View reviewed changes

kamijin-fanta force-pushed the redis-cluster branch from 2915141 to d75dac2 Compare April 14, 2021 08:37

pstibrany approved these changes Apr 14, 2021

View reviewed changes

pracucci reviewed Apr 16, 2021

View reviewed changes

kamijin-fanta force-pushed the redis-cluster branch 2 times, most recently from cb7c71e to 7d2ffa3 Compare April 28, 2021 00:41

pracucci approved these changes Apr 29, 2021

View reviewed changes

kamijin-fanta force-pushed the redis-cluster branch from 3e7280d to 34c9a9b Compare May 6, 2021 01:31

pull-request-size bot added size/XXL and removed size/M labels May 6, 2021

kamijin-fanta added 4 commits May 6, 2021 11:59

chunks/cache: redis cluster support

65e2a81

Signed-off-by: kamijin_fanta <[email protected]>

add redis client test

66507d3

Signed-off-by: kamijin_fanta <[email protected]>

update miniredis

132d092

Signed-off-by: kamijin_fanta <[email protected]>

redis_client: changed the test to simple.

afddc3f

Signed-off-by: kamijin_fanta <[email protected]>

kamijin-fanta force-pushed the redis-cluster branch from 34c9a9b to afddc3f Compare May 6, 2021 03:00

pracucci approved these changes May 13, 2021

View reviewed changes

Merge branch 'master' into redis-cluster

f3defda

pracucci enabled auto-merge (squash) May 27, 2021 09:04

pracucci merged commit cd7f60d into cortexproject:master May 27, 2021

	func TestSchemaConfig_Validate(t *testing.T) {
	t.Parallel()

	tests := map[string]struct {
	config *SchemaConfig
	expected *SchemaConfig
	err error

Fixed Cache fetch error on Redis Cluster #4056

Fixed Cache fetch error on Redis Cluster #4056

Uh oh!

Conversation

kamijin-fanta commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevesg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevesg left a comment

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci commented Apr 26, 2021

Uh oh!

gouthamve commented Apr 28, 2021

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamijin-fanta commented Apr 8, 2021 •

edited

Loading