read redis lock options from config to support custom TTL & timeout #1791

r-ashish · 2022-09-15T10:14:10Z

What is the problem I am trying to address?

Create RedisLockConfig to support specifying custom TTL & timeout values through config.
- Issue: For several of our internal modules it may take longer than 5 mins (current TTL default) to fetch them due to them being part of a huge mono-repository and consequently new concurrent requests for the same module end up re-acquiring the lock after the expiry, resulting in another stash operation.
- This may add extra, unnecessary load on the git server and concurrent writes in storage.
Update github.com/bsm/redislock from v0.4.2 to v0.8.0
- Issue: The current version being used (v0.4.2) doesn't allow you to set a custom timeout and sets the timeout & TTL from the same arg
- v0.8.0 supports setting custom timeouts independently (included in 0.7.2 onwards).Create RedisLockConfig to support specifying custom TTL & timeout values through config.

r-ashish · 2022-09-15T15:20:00Z

~~Marking as WIP:~~

~~need to look at the failing test.~~
~~[nitpick] think about splitting this into 2 PRs. one for the version upgrade and the other to add the new config.~~

config.dev.toml

r-ashish · 2022-09-16T01:59:07Z

~~fixed the test, one of the error msgs that the test was relying on has changed with the new version of redis client.~~
Not required anymore.

go.mod

…ersion

manugupt1 · 2022-09-16T04:44:22Z

config.dev.toml

@@ -320,6 +320,15 @@ IndexType = "none"
        # Password is the password for a redis SingleFlight lock.
        # Env override: ATHENS_REDIS_PASSWORD
        Password = ""
+
+        [SingleFlight.Redis.LockConfig]


We have config parsing tests can we update them?

Updated the tests.

manugupt1 · 2022-09-16T04:46:35Z

config.dev.toml

+            # Timeout for acquiring the lock in seconds. Defaults to 300 seconds (5 minutes).
+            Timeout = 300
+            # Max retries while acquiring the lock. Defaults to 300.
+            MaxRetries = 300


should this be 5?

It's currently set to 300 IINW so I used the same as the default value:

athens/pkg/stash/with_redis.go

Line 48 in 7d67ee7

RetryStrategy: lock.LimitRetry(lock.LinearBackoff(time.Second), 60*5),

Hmm! This does not seem right, Should we revisit these values and have saner defaults. A request can hang for ever.
cc @marwan-at-work

agreed, retrying 300 times is way too much.

Updated to 10.

manugupt1 · 2022-09-16T04:51:59Z

config.dev.toml

+
+        [SingleFlight.Redis.LockConfig]
+            # TTL for the lock in seconds. Defaults to 300 seconds (5 minutes).
+            TTL = 300


I am not understanding this completely, but is there a reason to have TTL and timeout configured separately?
Can you please update the documentation when these should be used?

My idea behind having two separate configurations for them:

TTL & timeout are two different things, i.e, TTL sets the expiry of the lock whereas timeout sets the context timeout and allows you to cancel retries after timeout.

For our use-case we may have higher TTL (15-20mins) but we wouldn't want retries to last that long.

So this might be helpful for those who would like to customize it in a similar way.

I'll check & update the relevant docs.

Updated the docs.

Regarding the following:
Timeout: Time to acquire locks should be in order of miliseconds or a few seconds not minutes.
I think we should have it to either 1 or 2.

I do not mind if you increase the TTL to 30 minutes as there maybe users with large modules.

That's true, yea it makes sense to reduce the timeout as well.

We also have retries and the timeout is not per retry so I think we should at least have enough room to allow the retries - retries are configured to be 1/ sec up-to 10 so that's roughly >=10s, so 10-20s should be good enough. I'll make the default 15s for now.

Updated timeout to 15s. Also updated default TTL to 15 mins.

manugupt1 · 2022-09-16T04:52:59Z

pkg/stash/with_redis.go

 	"github.com/gomods/athens/pkg/config"
 	"github.com/gomods/athens/pkg/errors"
 	"github.com/gomods/athens/pkg/observ"
 	"github.com/gomods/athens/pkg/storage"
 )

+const (


We should not need these anymore as the config should pass in the default value right, can we throw an error if the config is not passing default values.

Yea makes sense, will update this.

manugupt1 · 2022-09-21T04:26:18Z

config.dev.toml

+
+        [SingleFlight.Redis.LockConfig]
+            # TTL for the lock in seconds. Defaults to 300 seconds (5 minutes).
+            TTL = 300


Regarding the following:
Timeout: Time to acquire locks should be in order of miliseconds or a few seconds not minutes.
I think we should have it to either 1 or 2.

I do not mind if you increase the TTL to 30 minutes as there maybe users with large modules.

manugupt1 · 2022-09-21T04:28:05Z

config.dev.toml

        MasterName = "redis-1"
        # SentinelPassword is an optional password for authenticating with
        # redis sentinel
+        # Env override: ATHENS_REDIS_SENTINEL_PASSWORD


Thank you for updating the docs.

manugupt1 · 2022-09-29T05:53:00Z

thanks!

r-ashish requested a review from a team as a code owner September 15, 2022 10:14

r-ashish changed the title ~~[WIP] read redis lock options from config to support custom TTL & timeout~~ read redis lock options from config to support custom TTL & timeout Sep 15, 2022

r-ashish force-pushed the redis-lock-options branch from 8135342 to 8f4233c Compare September 15, 2022 11:12

r-ashish changed the title ~~read redis lock options from config to support custom TTL & timeout~~ [WIP] read redis lock options from config to support custom TTL & timeout Sep 15, 2022

arschles approved these changes Sep 15, 2022

View reviewed changes

config.dev.toml Outdated Show resolved Hide resolved

config.dev.toml Outdated Show resolved Hide resolved

r-ashish added 3 commits September 16, 2022 09:55

read redis lock options from config to support custom TTL & timeout

e39d20d

fix test

273c6e6

fix typo

c990dc6

r-ashish force-pushed the redis-lock-options branch from 7522682 to c990dc6 Compare September 16, 2022 01:55

r-ashish changed the title ~~[WIP] read redis lock options from config to support custom TTL & timeout~~ read redis lock options from config to support custom TTL & timeout Sep 16, 2022

swamvenk reviewed Sep 16, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

r-ashish added 2 commits September 16, 2022 12:02

downgrade to bsm/redislock@v0.7.2 to prevent usage of beta go-redis v…

1445326

…ersion

revert test changes

a08caf8

manugupt1 reviewed Sep 16, 2022

View reviewed changes

r-ashish added 5 commits September 20, 2022 17:47

return error for invalid lock config

c064ed4

update config parsing test

3609063

udpate docs to include redis lock config

018103f

fix test

6625c47

set default max retries to 10

c28f9ed

manugupt1 requested changes Sep 21, 2022

View reviewed changes

r-ashish added 2 commits September 23, 2022 21:06

reduce default redis lock timeout to 15s

fde6cfb

update default TTL to 15mins

90a8732

manugupt1 approved these changes Sep 27, 2022

View reviewed changes

Merge branch 'main' into redis-lock-options

c2ba2f4

manugupt1 merged commit cc496af into gomods:main Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read redis lock options from config to support custom TTL & timeout #1791

read redis lock options from config to support custom TTL & timeout #1791

r-ashish commented Sep 15, 2022 •

edited

Loading

r-ashish commented Sep 15, 2022 •

edited

Loading

r-ashish commented Sep 16, 2022 •

edited

Loading

manugupt1 Sep 16, 2022

r-ashish Sep 20, 2022

manugupt1 Sep 16, 2022

r-ashish Sep 17, 2022

manugupt1 Sep 17, 2022

marwan-at-work Sep 17, 2022

r-ashish Sep 20, 2022

manugupt1 Sep 16, 2022

r-ashish Sep 17, 2022

r-ashish Sep 17, 2022

r-ashish Sep 20, 2022

manugupt1 Sep 21, 2022

r-ashish Sep 23, 2022

r-ashish Sep 23, 2022 •

edited

Loading

manugupt1 Sep 16, 2022

r-ashish Sep 17, 2022

r-ashish Sep 20, 2022

manugupt1 Sep 21, 2022

manugupt1 Sep 21, 2022

manugupt1 commented Sep 29, 2022

read redis lock options from config to support custom TTL & timeout #1791

read redis lock options from config to support custom TTL & timeout #1791

Conversation

r-ashish commented Sep 15, 2022 • edited Loading

What is the problem I am trying to address?

r-ashish commented Sep 15, 2022 • edited Loading

r-ashish commented Sep 16, 2022 • 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

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

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

r-ashish Sep 23, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 commented Sep 29, 2022

r-ashish commented Sep 15, 2022 •

edited

Loading

r-ashish commented Sep 15, 2022 •

edited

Loading

r-ashish commented Sep 16, 2022 •

edited

Loading

r-ashish Sep 23, 2022 •

edited

Loading