Add redis sentinel support #161

cyrinux · 2023-12-11T15:13:59Z

Hi,

This PR do the following things:

Bump go-redis to v9
Support redis sentinel support

Thanks for the project.

cyrinux · 2023-12-12T10:30:13Z

It should pass the tests now 😃

sebmil-daily

Validated in test environment

README.md

guscarreon · 2023-12-12T21:34:46Z

README.md

-| port | integer | Redis server port |
+| host | string | Redis server URI (redis standalone mode) |
+| port | integer | Redis server port (redis standalone mode) |
+| hosts |string array | Redis server sentinel URI (redis sentinel mode)|


Both the name hosts and the description in the third column suggests that host and hosts have the same format but, the SentinelAddrs field is a list of host:port strings.

19 // FailoverOptions are used to configure a failover client and should 20 // be passed to NewFailoverClient. 21 type FailoverOptions struct { 22 // The master name. 23 MasterName string 24 // A seed list of host:port addresses of sentinel nodes. 25 SentinelAddrs []string 26 /newgo/pkg/mod/github.com/redis/go-redis/v9@v9.3.0/sentinel.go

Should we modify the description to mention the formatting difference with host? Or add a separate array called ports?

246 | Configuration field | Type | Description | 247 | --- | --- | --- | 248 | host | string | Redis server URI (redis standalone mode) | 249 | port | integer | Redis server port (redis standalone mode) | 250 | hosts |string array | Redis server sentinel URI (redis sentinel mode)| + | ports |integer array | Redis server sentinel port (redis sentinel mode)| 251 | mastername | string | Redis master sentinel name (redis sentinel mode)| README.md

I lean more towards modifiying the description to explain the difference and even rename this variable. I'm not great at naming. sentinel_hosts_ports maybe?

246 | Configuration field | Type | Description | 247 | --- | --- | --- | 248 | host | string | Redis server URI (redis standalone mode) | 249 | port | integer | Redis server port (redis standalone mode) | 250 - | hosts |string array | Redis server sentinel URI (redis sentinel mode)| + | sentinel_hosts_ports | string array | Redis server sentinel host:port list (redis sentinel mode)| 251 | mastername | string | Redis master sentinel name (redis sentinel mode)| README.md

Thoughts?

I will go for sentinel_hosts_ports and sentinel_mastername so. I'm agree with that.

I tried to do breaking change and want to keep host and port for redis mode. And in general in others backends, its common to use hosts that take a list of host:port.

config/backends.go

guscarreon · 2023-12-12T22:09:00Z

backends/config/config_test.go

@@ -149,6 +149,8 @@ func TestNewBaseBackend(t *testing.T) {
 			desc:     "Redis",
 			inConfig: config.Backend{Type: config.BackendRedis},
 			expectedLogEntries: []logEntry{
+				{msg: "Error creating Redis backend: At least one Host[s] is required.", lvl: logrus.FatalLevel},
+				{msg: "Creating Redis backend", lvl: logrus.InfoLevel},
 				{msg: "Error creating Redis backend: ", lvl: logrus.FatalLevel},


In practice, logrus.FatalLevel stops execution. Under this scenario, we would only see the first entry in the logs:

148 { 149 desc: "Redis", 150 inConfig: config.Backend{Type: config.BackendRedis}, 151 expectedLogEntries: []logEntry{ 152 {msg: "Error creating Redis backend: At least one Host[s] is required.", lvl: logrus.FatalLevel}, 153 - {msg: "Creating Redis backend", lvl: logrus.InfoLevel}, 154 - {msg: "Error creating Redis backend: ", lvl: logrus.FatalLevel}, 155 }, 156 }, backends/config/config_test.go

I thought the same but without those three items the test fail.

It's because the test function disables the logrus exit functions, so we can have as many log.Fatalf calls without interrupting the test:

func TestNewBaseBackend(t *testing.T) { // logrus entries will be recorded to this `hook` object so we can compare and assert them hook := logrusTest.NewGlobal() defer func() { logrus.StandardLogger().ExitFunc = nil }() logrus.StandardLogger().ExitFunc = func(int) {}

Good catch, thanks @sebmil-daily

guscarreon · 2023-12-12T22:16:14Z

config/config_test.go

@@ -1202,6 +1202,7 @@ func getExpectedDefaultConfig() Configuration {
 				DefaultTTL: utils.CASSANDRA_DEFAULT_TTL_SECONDS,
 			},
 			Redis: Redis{
+				Hosts:             []string{},


I don't think we need this. The test is able to pass without it.

I beat a lot to find what was wrong before I explicit declare this here.
Without, it fail because the array is nil.

bsardo · 2023-12-13T18:08:45Z

Hi @cyrinux, if you don't mind, going forward could you please avoid force pushes and instead just add additional commits when making updates? It saves us review time since we can then just review the delta. Thanks!

cyrinux · 2023-12-13T19:17:36Z

Hi @cyrinux, if you don't mind, going forward could you please avoid force pushes and instead just add additional commits when making updates? It saves us review time since we can then just review the delta. Thanks!

Sure, I understand no problems. I don't touch anymore now. Waiting for you now 😉

guscarreon

There's still an error in a test case, hence the validation didn't pass. Can we please fix?
Otherwise, it's looking pretty good.

guscarreon · 2023-12-13T21:41:23Z

config/backends.go

-	log.Infof("config.backend.redis.port: %d", cfg.Port)
+	if cfg.Host != "" && len(cfg.SentinelHostsPorts) > 0 {
+		log.Infof("config.backend.redis.sentinel_hosts_ports: %s. Note that redis 'host' will be ignore if 'sentinel_hosts_ports' is define", cfg.SentinelHostsPorts)
+		log.Infof("config.backend.redis.sentinel_mastername: %s.", cfg.SentinelMasterName)


@cyrinux thank you for your addresing our feedback. When we want to configure Redis in Sentinel mode, is SentinelMasterName a required field? In other words, if len(cfg.SentinelHostsPorts) > 0 is true, do we also need cfg.SentinelMasterName to not be empty?

If we do, then I believe an extra cfg.SentinelMasterName != "" check should be added in case len(cfg.SentinelHostsPorts) > 0 is found to be true and log.Infof() or error out accordingly.

Thoughts?

sebmil-daily · 2023-12-15T09:51:50Z

Hey @guscarreon , considering those points:

we have several duplicates in the configuration options between redis standalone and redis sentinel
even with the sentinel prefix, we are adding a lot of complexity in the redis backend configuration
it's not clear for the user which ones are mandatory in each case
there are more sentinel-specific options that we don't yet support
most of the redis backend code base is only for configuration and initialization

I suggest we cancel this PR for now and we'll come back with a new one that will introduce redis-sentinel as a whole new backend. It will ensure we are not creating any risk with the current redis (standalone) backend and it'll be more clear both for the users and for future development to have a separated configuration structure and validation.

What do you think?

(@cyrinux and me are working in the same company and we already agreed on this)

guscarreon · 2023-12-18T17:15:28Z

I suggest we cancel this PR for now and we'll come back with a new one that will introduce redis-sentinel as a whole new backend

I agree. Let's close this PR now and will keep an eye for your upcoming Redis entinel adapter PR.

cyrinux force-pushed the feat/support-redis-sentinel branch 3 times, most recently from 632ced9 to deb938c Compare December 11, 2023 15:20

cyrinux changed the title ~~feat: add redis sentinel support~~ feat: add redis sentinel support 5WIP) Dec 11, 2023

cyrinux changed the title ~~feat: add redis sentinel support 5WIP)~~ feat: add redis sentinel support (WIP) Dec 11, 2023

cyrinux force-pushed the feat/support-redis-sentinel branch 2 times, most recently from 0139e4d to 0a0db31 Compare December 11, 2023 15:30

cyrinux changed the title ~~feat: add redis sentinel support (WIP)~~ feat: add redis sentinel support Dec 11, 2023

bsardo assigned guscarreon Dec 11, 2023

bsardo changed the title ~~feat: add redis sentinel support~~ Add redis sentinel support Dec 11, 2023

cyrinux force-pushed the feat/support-redis-sentinel branch 4 times, most recently from 518074e to 88d9616 Compare December 12, 2023 10:04

cyrinux force-pushed the feat/support-redis-sentinel branch from 88d9616 to b418372 Compare December 12, 2023 12:38

feat: add redis sentinel support

366fd2d

cyrinux force-pushed the feat/support-redis-sentinel branch from b418372 to 366fd2d Compare December 12, 2023 12:55

sebmil-daily approved these changes Dec 12, 2023

View reviewed changes

guscarreon reviewed Dec 12, 2023

View reviewed changes

cyrinux force-pushed the feat/support-redis-sentinel branch from dfd0ac3 to b9f0ed2 Compare December 13, 2023 07:33

cyrinux requested review from guscarreon and sebmil-daily December 13, 2023 07:34

chore: review 1

6cbdc16

cyrinux force-pushed the feat/support-redis-sentinel branch from b9f0ed2 to 6cbdc16 Compare December 13, 2023 07:52

guscarreon reviewed Dec 13, 2023

View reviewed changes

cyrinux closed this Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redis sentinel support #161

Add redis sentinel support #161

cyrinux commented Dec 11, 2023 •

edited

Loading

cyrinux commented Dec 12, 2023

sebmil-daily left a comment

guscarreon Dec 12, 2023

cyrinux Dec 13, 2023

guscarreon Dec 12, 2023

cyrinux Dec 13, 2023

sebmil-daily Dec 13, 2023 •

edited

Loading

cyrinux Dec 13, 2023

guscarreon Dec 12, 2023

cyrinux Dec 13, 2023

bsardo commented Dec 13, 2023

cyrinux commented Dec 13, 2023 •

edited

Loading

guscarreon left a comment

guscarreon Dec 13, 2023

sebmil-daily commented Dec 15, 2023 •

edited

Loading

guscarreon commented Dec 18, 2023

Add redis sentinel support #161

Add redis sentinel support #161

Conversation

cyrinux commented Dec 11, 2023 • edited Loading

cyrinux commented Dec 12, 2023

sebmil-daily left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmil-daily Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Dec 13, 2023

cyrinux commented Dec 13, 2023 • edited Loading

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmil-daily commented Dec 15, 2023 • edited Loading

guscarreon commented Dec 18, 2023

cyrinux commented Dec 11, 2023 •

edited

Loading

sebmil-daily Dec 13, 2023 •

edited

Loading

cyrinux commented Dec 13, 2023 •

edited

Loading

sebmil-daily commented Dec 15, 2023 •

edited

Loading