Add MaxRetry for MinIO operations #1530
Conversation
itpey
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes introduced in this pull request add a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config
participant Storage
User->>Config: Set MaxRetry
Config->>Storage: Create new instance with MaxRetry
Storage->>Storage: Perform operations (Get, Set, etc.)
Storage-->>User: Return results
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
minio/config.go (1)
Line range hint
77-93: Add MaxRetry handling to configDefault function.The configDefault helper function should handle the MaxRetry field to ensure it gets the default value when not explicitly set.
Apply this diff:
func configDefault(config ...Config) Config { // Return default config if nothing provided if len(config) < 1 { return ConfigDefault } // Override default config cfg := config[0] // Set default values if cfg.Bucket == "" { cfg.Bucket = ConfigDefault.Bucket } + if cfg.MaxRetry == 0 { + cfg.MaxRetry = ConfigDefault.MaxRetry + } return cfg }minio/minio.go (1)
Line range hint
31-47: Consider returning errors instead of panickingWhile not directly related to the retry changes, the
Newfunction currently panics on initialization errors. As a library function, it would be better to return errors and let the caller decide how to handle them.Consider refactoring the function signature to:
func New(config ...Config) (*Storage, error) { // Set default config cfg := configDefault(config...) // Minio instance minioClient, err := minio.New(cfg.Endpoint, &minio.Options{ Creds: credentials.NewStaticV4(cfg.Credentials.AccessKeyID, cfg.Credentials.SecretAccessKey, cfg.Token), Secure: cfg.Secure, Region: cfg.Region, Transport: &minio.Transport{ MaxRetryAttempts: cfg.MaxRetry, }, }) if err != nil { return nil, fmt.Errorf("failed to create MinIO client: %w", err) } storage := &Storage{minio: minioClient, cfg: cfg, ctx: context.Background()} // Reset all entries if set to true if cfg.Reset { if err = storage.Reset(); err != nil { return nil, fmt.Errorf("failed to reset storage: %w", err) } } // check bucket if err = storage.CheckBucket(); err != nil { // create bucket if err = storage.CreateBucket(); err != nil { return nil, fmt.Errorf("failed to create bucket: %w", err) } } return storage, nil }minio/README.md (1)
97-100: Fix indentation: Replace hard tabs with spacesThe indentation uses hard tabs instead of spaces. To maintain consistency with the rest of the documentation, please replace the hard tabs with spaces.
Apply this diff:
// The maximum number of times requests that encounter retryable failures should be attempted. - // Optional. Default is 10, same as the MinIO client. - MaxRetry int + // Optional. Default is 10, same as the MinIO client. + MaxRetry int🧰 Tools
🪛 Markdownlint
98-98: Column: 1
Hard tabs(MD010, no-hard-tabs)
99-99: Column: 1
Hard tabs(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
minio/README.md(2 hunks)minio/config.go(2 hunks)minio/minio.go(1 hunks)
🧰 Additional context used
🪛 Markdownlint
minio/README.md
98-98: Column: 1
Hard tabs
(MD010, no-hard-tabs)
99-99: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (2)
minio/config.go (1)
32-34: LGTM! Well-documented MaxRetry field.
The new MaxRetry field is well-documented with clear comments indicating its purpose, optional nature, and default value.
minio/README.md (1)
131-131:
Add MaxRetry field to ConfigDefault
The MaxRetry field is documented as having a default value of 10, but it's missing from the ConfigDefault struct. Please add it to maintain consistency with the documentation.
Add the following line to the ConfigDefault struct:
var ConfigDefault = Config{
Bucket: "fiber-bucket",
Endpoint: "",
Region: "",
Token: "",
Secure: false,
Reset: false,
+ MaxRetry: 10,
Credentials: Credentials{},Likely invalid or redundant comment.
|
Thx for the pr Pls add a test case |
I don't think a new test case is necessary since this feature is already covered and well-tested within the MINIO client itself. I have not implemented a new retry mechanism. https://github.com/minio/minio-go/blob/ab1c2fe050a4dd502c863178696a54c44610ca82/retry.go |
this should not be used to check the underlying functionality, but only to check that the configuration has been set correctly this prevents an error from occurring later if the configurations become much larger |
|
but it looks like I'm a bit too strict there |
This PR introduces a new MaxRetry configuration option to the Config struct, allowing users to specify the maximum number of retry attempts for MinIO operations.
Summary by CodeRabbit
New Features
MaxRetryconfiguration option to specify the maximum retry attempts for requests, enhancing reliability during failures.MaxRetryfield.Improvements
MaxRetryparameter when creating new storage instances, allowing for better customization.