-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat: Add retry mechanism for storage initialization #3701
base: master
Are you sure you want to change the base?
Conversation
e050033
to
b1762c8
Compare
cdb76ff
to
6ad2c32
Compare
a091d04
to
eb7b854
Compare
2b5b88b
to
8adc2c4
Compare
Signed-off-by: moeneuron <[email protected]>
Signed-off-by: moeneuron <[email protected]>
Signed-off-by: moeneuron <[email protected]>
8adc2c4
to
255cce4
Compare
@nabokihms I would like to request a review here, if possible! Or how do you like to plan this in the next release? |
cmd/dex/serve.go
Outdated
retryAttempts := storageConfig.RetryAttempts | ||
if retryAttempts == 0 { | ||
retryAttempts = 5 // Default to 5 attempts | ||
} | ||
|
||
retryDelay, err := time.ParseDuration(storageConfig.RetryDelay) | ||
if err != nil { | ||
retryDelay = 5 * time.Second // Default to 5 seconds | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this code to the config parsing, we usually enforce config default there, not in the method. I expect it to be in cmd/dex/config.go
cmd/dex/config.go
Outdated
RetryAttempts int `json:"retryAttempts"` | ||
RetryDelay string `json:"retryDelay"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Probably it is better to use token/bucket algorithm or an exponential backoff.
- Let's put options under a single key
retry:
attempts: 3
delay: 5s
cmd/dex/config.go
Outdated
Type string `json:"type"` | ||
Config json.RawMessage `json:"config"` | ||
RetryAttempts int `json:"retryAttempts"` | ||
RetryDelay string `json:"retryDelay"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to validate here that delay is in the go time format, e.g., 5s
, 10m
?
9161a5f
to
5baca04
Compare
connected #1189 |
Resolves #3602
PR includes: