Skip to content
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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MoeBensu
Copy link
Contributor

@MoeBensu MoeBensu commented Aug 17, 2024

Resolves #3602

PR includes:

  • Implement configurable retry logic for storage connection attempts
  • Add configuration options for retry attempts and delay
  • Default to 5 attempts with 5 second delay if not specified

@MoeBensu MoeBensu force-pushed the feat/add-storage-retry-mechanism branch from e050033 to b1762c8 Compare August 17, 2024 20:04
@MoeBensu MoeBensu marked this pull request as draft August 17, 2024 20:11
@MoeBensu MoeBensu force-pushed the feat/add-storage-retry-mechanism branch 7 times, most recently from cdb76ff to 6ad2c32 Compare August 18, 2024 14:34
@MoeBensu MoeBensu changed the title Add retry mechanism for storage initialization feat: Add retry mechanism for storage initialization Aug 18, 2024
@MoeBensu MoeBensu force-pushed the feat/add-storage-retry-mechanism branch from a091d04 to eb7b854 Compare August 18, 2024 14:37
@MoeBensu MoeBensu marked this pull request as ready for review August 18, 2024 14:42
@MoeBensu MoeBensu force-pushed the feat/add-storage-retry-mechanism branch from 2b5b88b to 8adc2c4 Compare August 20, 2024 16:28
@MoeBensu MoeBensu force-pushed the feat/add-storage-retry-mechanism branch from 8adc2c4 to 255cce4 Compare August 20, 2024 16:34
@MoeBensu
Copy link
Contributor Author

@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
Comment on lines 698 to 706
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
}
Copy link
Member

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

Comment on lines 259 to 260
RetryAttempts int `json:"retryAttempts"`
RetryDelay string `json:"retryDelay"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Probably it is better to use token/bucket algorithm or an exponential backoff.
  2. Let's put options under a single key
retry:
  attempts: 3
  delay: 5s

Type string `json:"type"`
Config json.RawMessage `json:"config"`
RetryAttempts int `json:"retryAttempts"`
RetryDelay string `json:"retryDelay"`
Copy link
Member

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?

@MoeBensu MoeBensu marked this pull request as draft September 3, 2024 21:28
@MoeBensu MoeBensu force-pushed the feat/add-storage-retry-mechanism branch from 9161a5f to 5baca04 Compare September 4, 2024 18:52
@nabokihms
Copy link
Member

connected #1189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dex doesn't retry when database not available during start
2 participants