From 7a2c77ef1a4478bd0e7c8a6c3244d498fae19fb2 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Tue, 18 Jun 2024 11:17:55 +0530 Subject: [PATCH 01/11] feat: fallback MGetCache to MGet --- helper.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/helper.go b/helper.go index 18ca2938..4abe5ee3 100644 --- a/helper.go +++ b/helper.go @@ -46,6 +46,9 @@ func MGetCache(client Client, ctx context.Context, ttl time.Duration, keys []str if len(keys) == 0 { return make(map[string]RedisMessage), nil } + if isCacheDisabled(client) { + return MGet(client, ctx, keys) + } cmds := mgetcachecmdsp.Get(len(keys), len(keys)) defer mgetcachecmdsp.Put(cmds) for i := range cmds.s { @@ -54,6 +57,22 @@ func MGetCache(client Client, ctx context.Context, ttl time.Duration, keys []str return doMultiCache(client, ctx, cmds.s, keys) } +func isCacheDisabled(client Client) bool { + switch c := client.(type) { + case *singleClient: + // what do we do here ? @rueian + return false + case *sentinelClient: + if c.mOpt != nil && c.mOpt.DisableCache { + return true + } + if c.sOpt != nil && c.sOpt.DisableCache { + return true + } + } + return false +} + // MGet is a helper that consults the redis directly with multiple keys by grouping keys within same slot into MGET or multiple GETs func MGet(client Client, ctx context.Context, keys []string) (ret map[string]RedisMessage, err error) { if len(keys) == 0 { From 157c8e132983ab9ed7d69343a7be83cb508cb9f7 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Wed, 19 Jun 2024 00:36:45 +0530 Subject: [PATCH 02/11] feat: update struct --- client.go | 15 ++++++++------- cluster.go | 6 +++++- helper.go | 13 ++++++------- sentinel.go | 6 +++++- 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/client.go b/client.go index e3a79213..5eee918c 100644 --- a/client.go +++ b/client.go @@ -10,10 +10,11 @@ import ( ) type singleClient struct { - conn conn - stop uint32 - cmd Builder - retry bool + conn conn + stop uint32 + cmd Builder + retry bool + DisableCache bool } func newSingleClient(opt *ClientOption, prev conn, connFn connFn) (*singleClient, error) { @@ -30,11 +31,11 @@ func newSingleClient(opt *ClientOption, prev conn, connFn connFn) (*singleClient if err := conn.Dial(); err != nil { return nil, err } - return newSingleClientWithConn(conn, cmds.NewBuilder(cmds.NoSlot), !opt.DisableRetry), nil + return newSingleClientWithConn(conn, cmds.NewBuilder(cmds.NoSlot), !opt.DisableRetry, opt.DisableCache), nil } -func newSingleClientWithConn(conn conn, builder Builder, retry bool) *singleClient { - return &singleClient{cmd: builder, conn: conn, retry: retry} +func newSingleClientWithConn(conn conn, builder Builder, retry, disableCache bool) *singleClient { + return &singleClient{cmd: builder, conn: conn, retry: retry, DisableCache: disableCache} } func (c *singleClient) B() Builder { diff --git a/cluster.go b/cluster.go index a5ec8e66..11ca5f7e 100644 --- a/cluster.go +++ b/cluster.go @@ -1053,8 +1053,12 @@ func (c *clusterClient) Dedicate() (DedicatedClient, func()) { func (c *clusterClient) Nodes() map[string]Client { c.mu.RLock() nodes := make(map[string]Client, len(c.conns)) + disableCache := false + if (c.opt != nil && c.opt.DisableCache) || (c.rOpt != nil && c.rOpt.DisableCache) { + disableCache = true + } for addr, cc := range c.conns { - nodes[addr] = newSingleClientWithConn(cc.conn, c.cmd, c.retry) + nodes[addr] = newSingleClientWithConn(cc.conn, c.cmd, c.retry, disableCache) } c.mu.RUnlock() return nodes diff --git a/helper.go b/helper.go index 4abe5ee3..0c95301e 100644 --- a/helper.go +++ b/helper.go @@ -60,15 +60,14 @@ func MGetCache(client Client, ctx context.Context, ttl time.Duration, keys []str func isCacheDisabled(client Client) bool { switch c := client.(type) { case *singleClient: - // what do we do here ? @rueian - return false - case *sentinelClient: - if c.mOpt != nil && c.mOpt.DisableCache { - return true - } - if c.sOpt != nil && c.sOpt.DisableCache { + if c.DisableCache { return true } + return false + case *sentinelClient: + return (c.mOpt != nil && c.mOpt.DisableCache) || (c.sOpt != nil && c.sOpt.DisableCache) + case *clusterClient: + return (c.opt != nil && c.opt.DisableCache) || (c.rOpt != nil && c.rOpt.DisableCache) } return false } diff --git a/sentinel.go b/sentinel.go index f4e716ee..b1b52aea 100644 --- a/sentinel.go +++ b/sentinel.go @@ -177,7 +177,11 @@ func (c *sentinelClient) Dedicate() (DedicatedClient, func()) { func (c *sentinelClient) Nodes() map[string]Client { conn := c.mConn.Load().(conn) - return map[string]Client{conn.Addr(): newSingleClientWithConn(conn, c.cmd, c.retry)} + disableCache := false + if (c.mOpt != nil && c.mOpt.DisableCache) || (c.sOpt != nil && c.sOpt.DisableCache) { + disableCache = true + } + return map[string]Client{conn.Addr(): newSingleClientWithConn(conn, c.cmd, c.retry, disableCache)} } func (c *sentinelClient) Close() { From 61eeffa831b1955f53afd75c032babe33ef46857 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Wed, 19 Jun 2024 18:49:35 +0530 Subject: [PATCH 03/11] feat: address comments --- cluster.go | 5 +---- helper_test.go | 11 +++++++++++ sentinel.go | 5 +---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/cluster.go b/cluster.go index 11ca5f7e..e0b0393d 100644 --- a/cluster.go +++ b/cluster.go @@ -1053,10 +1053,7 @@ func (c *clusterClient) Dedicate() (DedicatedClient, func()) { func (c *clusterClient) Nodes() map[string]Client { c.mu.RLock() nodes := make(map[string]Client, len(c.conns)) - disableCache := false - if (c.opt != nil && c.opt.DisableCache) || (c.rOpt != nil && c.rOpt.DisableCache) { - disableCache = true - } + disableCache := c.opt != nil && c.opt.DisableCache for addr, cc := range c.conns { nodes[addr] = newSingleClientWithConn(cc.conn, c.cmd, c.retry, disableCache) } diff --git a/helper_test.go b/helper_test.go index dcb1e17f..38676f14 100644 --- a/helper_test.go +++ b/helper_test.go @@ -18,6 +18,17 @@ func TestMGetCache(t *testing.T) { if err != nil { t.Fatalf("unexpected err %v", err) } + disableCacheClient, err := newSingleClient(&ClientOption{InitAddress: []string{""}, DisableCache: true}, m, func(dst string, opt *ClientOption) conn { + return m + }) + if err != nil { + t.Fatalf("unexpected err %v", err) + } + t.Run("DisableCache", func(t *testing.T) { + if v, err := MGetCache(disableCacheClient, context.Background(), 100, []string{"1", "2"}); err != nil || v == nil { + t.Fatalf("unexpected response %v %v", v, err) + } + }) t.Run("Delegate DoCache", func(t *testing.T) { m.DoMultiCacheFn = func(multi ...CacheableTTL) *redisresults { if reflect.DeepEqual(multi[0].Cmd.Commands(), []string{"GET", "1"}) && multi[0].TTL == 100 && diff --git a/sentinel.go b/sentinel.go index b1b52aea..3333d4ae 100644 --- a/sentinel.go +++ b/sentinel.go @@ -177,10 +177,7 @@ func (c *sentinelClient) Dedicate() (DedicatedClient, func()) { func (c *sentinelClient) Nodes() map[string]Client { conn := c.mConn.Load().(conn) - disableCache := false - if (c.mOpt != nil && c.mOpt.DisableCache) || (c.sOpt != nil && c.sOpt.DisableCache) { - disableCache = true - } + disableCache := c.mOpt != nil && c.mOpt.DisableCache return map[string]Client{conn.Addr(): newSingleClientWithConn(conn, c.cmd, c.retry, disableCache)} } From 7ab96e317f6f6ccea459cd05db1f95107cfde225 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Wed, 19 Jun 2024 19:30:31 +0530 Subject: [PATCH 04/11] feat: address comments --- helper_test.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/helper_test.go b/helper_test.go index 38676f14..dcb1e17f 100644 --- a/helper_test.go +++ b/helper_test.go @@ -18,17 +18,6 @@ func TestMGetCache(t *testing.T) { if err != nil { t.Fatalf("unexpected err %v", err) } - disableCacheClient, err := newSingleClient(&ClientOption{InitAddress: []string{""}, DisableCache: true}, m, func(dst string, opt *ClientOption) conn { - return m - }) - if err != nil { - t.Fatalf("unexpected err %v", err) - } - t.Run("DisableCache", func(t *testing.T) { - if v, err := MGetCache(disableCacheClient, context.Background(), 100, []string{"1", "2"}); err != nil || v == nil { - t.Fatalf("unexpected response %v %v", v, err) - } - }) t.Run("Delegate DoCache", func(t *testing.T) { m.DoMultiCacheFn = func(multi ...CacheableTTL) *redisresults { if reflect.DeepEqual(multi[0].Cmd.Commands(), []string{"GET", "1"}) && multi[0].TTL == 100 && From 3e4d713bc085f4340b3298862d08883a92cea6c2 Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Thu, 20 Jun 2024 18:19:53 +0530 Subject: [PATCH 05/11] fix : apply suggestions from code review Co-authored-by: Rueian --- helper.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/helper.go b/helper.go index 0c95301e..419ce88e 100644 --- a/helper.go +++ b/helper.go @@ -60,14 +60,11 @@ func MGetCache(client Client, ctx context.Context, ttl time.Duration, keys []str func isCacheDisabled(client Client) bool { switch c := client.(type) { case *singleClient: - if c.DisableCache { - return true - } - return false + return c.DisableCache case *sentinelClient: - return (c.mOpt != nil && c.mOpt.DisableCache) || (c.sOpt != nil && c.sOpt.DisableCache) + return (c.mOpt != nil && c.mOpt.DisableCache) case *clusterClient: - return (c.opt != nil && c.opt.DisableCache) || (c.rOpt != nil && c.rOpt.DisableCache) + return (c.opt != nil && c.opt.DisableCache) } return false } From facc37cf9e9a517ad03bd5b6038ff268176669ee Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Thu, 20 Jun 2024 23:14:54 +0530 Subject: [PATCH 06/11] feat: add test for disabled cache --- helper.go | 4 ++-- helper_test.go | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/helper.go b/helper.go index 419ce88e..70339134 100644 --- a/helper.go +++ b/helper.go @@ -62,9 +62,9 @@ func isCacheDisabled(client Client) bool { case *singleClient: return c.DisableCache case *sentinelClient: - return (c.mOpt != nil && c.mOpt.DisableCache) + return c.mOpt != nil && c.mOpt.DisableCache case *clusterClient: - return (c.opt != nil && c.opt.DisableCache) + return c.opt != nil && c.opt.DisableCache } return false } diff --git a/helper_test.go b/helper_test.go index dcb1e17f..e2a03669 100644 --- a/helper_test.go +++ b/helper_test.go @@ -18,6 +18,23 @@ func TestMGetCache(t *testing.T) { if err != nil { t.Fatalf("unexpected err %v", err) } + disabledCacheClient, err := newSingleClient(&ClientOption{InitAddress: []string{""}, DisableCache: true}, m, func(dst string, opt *ClientOption) conn { + return m + }) + if err != nil { + t.Fatalf("unexpected err %v", err) + } + t.Run("Delegate DisabledCache MGetCache", func(t *testing.T) { + m.DoFn = func(cmd Completed) RedisResult { + if !reflect.DeepEqual(cmd.Commands(), []string{"MGET", "1", "2"}) { + t.Fatalf("unexpected command %v", cmd) + } + return newResult(RedisMessage{typ: '*', values: []RedisMessage{{typ: '+', string: "1"}, {typ: '+', string: "2"}}}, nil) + } + if v, err := MGetCache(disabledCacheClient, context.Background(), 100, []string{"1", "2"}); err != nil || v == nil { + t.Fatalf("unexpected response %v %v", v, err) + } + }) t.Run("Delegate DoCache", func(t *testing.T) { m.DoMultiCacheFn = func(multi ...CacheableTTL) *redisresults { if reflect.DeepEqual(multi[0].Cmd.Commands(), []string{"GET", "1"}) && multi[0].TTL == 100 && From 9b8d8c1cb56788d3037efd47e5481e72d122ccfb Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Thu, 20 Jun 2024 23:24:23 +0530 Subject: [PATCH 07/11] feat: add placeholder for sentinel client test --- helper_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/helper_test.go b/helper_test.go index e2a03669..dafbfd88 100644 --- a/helper_test.go +++ b/helper_test.go @@ -125,6 +125,35 @@ func TestMGetCache(t *testing.T) { } }) }) + // TODO: @SoulPancake I have some doubts about this @rueian + //t.Run("sentinel client", func(t *testing.T) { + // m := &mockConn{ + // DoFn: func(cmd Completed) RedisResult { + // return slotsResp + // }, + // } + // sentinelClient, err := newSentinelClient(&ClientOption{ + // InitAddress: []string{":0"}, + // Sentinel: SentinelOption{MasterSet: "masters"}, + // DisableCache: true, + // }, func(dst string, opt *ClientOption) conn { + // return m + // }) + // if err != nil { + // t.Fatalf("unexpected err %v", err) + // } + // t.Run("Delegate DisabledCache MGetCache", func(t *testing.T) { + // m.DoFn = func(cmd Completed) RedisResult { + // if !reflect.DeepEqual(cmd.Commands(), []string{"MGET", "1", "2"}) { + // t.Fatalf("unexpected command %v", cmd) + // } + // return newResult(RedisMessage{typ: '*', values: []RedisMessage{{typ: '+', string: "1"}, {typ: '+', string: "2"}}}, nil) + // } + // if v, err := MGetCache(sentinelClient, context.Background(), 100, []string{"1", "2"}); err != nil || v == nil { + // t.Fatalf("unexpected response %v %v", v, err) + // } + // }) + //}) } //gocyclo:ignore From 12d2467aaa7f034622a1909fa993a21956280c98 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Sat, 22 Jun 2024 01:42:12 +0530 Subject: [PATCH 08/11] feat: covering sentinel client disabledCache scenario --- helper_test.go | 29 ----------------------------- sentinel_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/helper_test.go b/helper_test.go index dafbfd88..e2a03669 100644 --- a/helper_test.go +++ b/helper_test.go @@ -125,35 +125,6 @@ func TestMGetCache(t *testing.T) { } }) }) - // TODO: @SoulPancake I have some doubts about this @rueian - //t.Run("sentinel client", func(t *testing.T) { - // m := &mockConn{ - // DoFn: func(cmd Completed) RedisResult { - // return slotsResp - // }, - // } - // sentinelClient, err := newSentinelClient(&ClientOption{ - // InitAddress: []string{":0"}, - // Sentinel: SentinelOption{MasterSet: "masters"}, - // DisableCache: true, - // }, func(dst string, opt *ClientOption) conn { - // return m - // }) - // if err != nil { - // t.Fatalf("unexpected err %v", err) - // } - // t.Run("Delegate DisabledCache MGetCache", func(t *testing.T) { - // m.DoFn = func(cmd Completed) RedisResult { - // if !reflect.DeepEqual(cmd.Commands(), []string{"MGET", "1", "2"}) { - // t.Fatalf("unexpected command %v", cmd) - // } - // return newResult(RedisMessage{typ: '*', values: []RedisMessage{{typ: '+', string: "1"}, {typ: '+', string: "2"}}}, nil) - // } - // if v, err := MGetCache(sentinelClient, context.Background(), 100, []string{"1", "2"}); err != nil || v == nil { - // t.Fatalf("unexpected response %v %v", v, err) - // } - // }) - //}) } //gocyclo:ignore diff --git a/sentinel_test.go b/sentinel_test.go index 01fb9e76..14008cc9 100644 --- a/sentinel_test.go +++ b/sentinel_test.go @@ -694,6 +694,46 @@ func TestSentinelClientDelegate(t *testing.T) { } defer client.Close() + disabledCacheClient, err := newSentinelClient(&ClientOption{InitAddress: []string{":0"}, DisableCache: true}, func(dst string, opt *ClientOption) conn { + if dst == ":0" { + return s0 + } + if dst == ":1" { + return m + } + return nil + }) + if err != nil { + t.Fatalf("unexpected err %v", err) + } + defer disabledCacheClient.Close() + + t.Run("Delegate MGetCache", func(t *testing.T) { + keys := []string{"key1", "key2"} + expectedCommand := []string{"MGET", "key1", "key2"} + + m.DoMultiFn = func(cmd ...Completed) *redisresults { + if !reflect.DeepEqual(cmd[0].Commands(), expectedCommand) { + t.Fatalf("unexpected command %v", cmd) + } + return &redisresults{s: []RedisResult{ + newResult(RedisMessage{typ: '+', string: "master"}, nil), + }} + } + + ret, err := MGetCache(disabledCacheClient, context.Background(), time.Second, keys) + if err != nil { + t.Fatalf("unexpected error %v", err) + } + + expected := map[string]RedisMessage{ + "key1": {typ: '+', string: "master"}, + } + if !reflect.DeepEqual(ret, expected) { + t.Fatalf("unexpected result %v, expected %v", ret, expected) + } + }) + t.Run("Nodes", func(t *testing.T) { if nodes := client.Nodes(); len(nodes) != 1 || nodes[":1"] == nil { t.Fatalf("unexpected nodes") From e3cc0ff86dc135cc4e5588f45ca2a3fad9a06995 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Mon, 24 Jun 2024 10:26:57 +0530 Subject: [PATCH 09/11] feat: add cluster client test --- helper_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/helper_test.go b/helper_test.go index e2a03669..af3a31a9 100644 --- a/helper_test.go +++ b/helper_test.go @@ -72,6 +72,9 @@ func TestMGetCache(t *testing.T) { DoFn: func(cmd Completed) RedisResult { return slotsResp }, + DoMultiFn: func(cmd ...Completed) *redisresults { + return &redisresults{s: []RedisResult{newErrResult(context.Canceled), newErrResult(context.Canceled)}} + }, } client, err := newClusterClient(&ClientOption{InitAddress: []string{":0"}}, func(dst string, opt *ClientOption) conn { return m @@ -79,6 +82,39 @@ func TestMGetCache(t *testing.T) { if err != nil { t.Fatalf("unexpected err %v", err) } + disabledCacheClient, err := newClusterClient(&ClientOption{InitAddress: []string{":0"}, DisableCache: true}, func(dst string, opt *ClientOption) conn { + return m + }) + if err != nil { + t.Fatalf("unexpected err %v", err) + } + t.Run("Delegate DisabledCache DoCache", func(t *testing.T) { + keys := make([]string, 100) + for i := range keys { + keys[i] = strconv.Itoa(i) + } + m.DoMultiCacheFn = func(multi ...CacheableTTL) *redisresults { + result := make([]RedisResult, len(multi)) + for i, key := range keys { + if !reflect.DeepEqual(multi[i].Cmd.Commands(), []string{"GET", key}) || multi[i].TTL != 100 { + t.Fatalf("unexpected command %v", multi) + return nil + } + result[i] = newResult(RedisMessage{typ: '+', string: key}, nil) + } + return &redisresults{s: result} + } + v, err := MGetCache(disabledCacheClient, context.Background(), 100, keys) + if err != nil { + t.Fatalf("unexpected response %v %v", v, err) + } + for _, key := range keys { + if v[key].string != key { + t.Fatalf("unexpected response %v", v) + } + } + }) + t.Run("Delegate DoCache", func(t *testing.T) { keys := make([]string, 100) for i := range keys { From 862c81a1d888983761a3c5df2b90ee40a3eb7bbc Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Mon, 24 Jun 2024 18:48:16 +0530 Subject: [PATCH 10/11] fix: test case --- helper_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helper_test.go b/helper_test.go index af3a31a9..de9f5f93 100644 --- a/helper_test.go +++ b/helper_test.go @@ -93,11 +93,11 @@ func TestMGetCache(t *testing.T) { for i := range keys { keys[i] = strconv.Itoa(i) } - m.DoMultiCacheFn = func(multi ...CacheableTTL) *redisresults { - result := make([]RedisResult, len(multi)) + m.DoMultiFn = func(cmd ...Completed) *redisresults { + result := make([]RedisResult, len(cmd)) for i, key := range keys { - if !reflect.DeepEqual(multi[i].Cmd.Commands(), []string{"GET", key}) || multi[i].TTL != 100 { - t.Fatalf("unexpected command %v", multi) + if !reflect.DeepEqual(cmd[i].Commands(), []string{"GET", key}) { + t.Fatalf("unexpected command %v", cmd) return nil } result[i] = newResult(RedisMessage{typ: '+', string: key}, nil) From 274ef6f4832901e0112033fe5dfa8d1eb60885a0 Mon Sep 17 00:00:00 2001 From: Anuragkillswitch <70265851+Anuragkillswitch@users.noreply.github.com> Date: Tue, 25 Jun 2024 00:09:47 +0530 Subject: [PATCH 11/11] feat: remove mockConn DoMultiFn --- helper_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/helper_test.go b/helper_test.go index de9f5f93..55ead3d1 100644 --- a/helper_test.go +++ b/helper_test.go @@ -72,9 +72,6 @@ func TestMGetCache(t *testing.T) { DoFn: func(cmd Completed) RedisResult { return slotsResp }, - DoMultiFn: func(cmd ...Completed) *redisresults { - return &redisresults{s: []RedisResult{newErrResult(context.Canceled), newErrResult(context.Canceled)}} - }, } client, err := newClusterClient(&ClientOption{InitAddress: []string{":0"}}, func(dst string, opt *ClientOption) conn { return m