Add retry to Validate #411
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
| if retryer.ShouldWaitAndRetry(ctx, err) { | ||
| continue | ||
| } | ||
| l.Error("error: validate failed", zap.Error(err)) |
There was a problem hiding this comment.
I think this log can be removed, as the error will be logged by the caller. Also you can remove error: from the fmt.Errorf().
There was a problem hiding this comment.
@ggreer Thanks! I was trying to keep it consistent with Grant and Revoke, but sounds like I can just update all three to remove the log and error:. 🙇
There was a problem hiding this comment.
Sounds good. I guess try granting or revoking to see how the error message differs.
There was a problem hiding this comment.
@ggreer I'm going to update this in all three functions.... agreed we don't need it after looking at logs.
mbburch
force-pushed
the
mbburch/bb-1181-retry-validate
branch
from
2336af6 to
aa341f0
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/connectorbuilder/connectorbuilder.go (1)
976-1003: Add metrics recording for consistency.The
Validatemethod is missing the task timing and metrics recording that other methods in this file use. This makes monitoring and debugging more difficult.Add metrics recording similar to other methods:
func (b *builderImpl) Validate(ctx context.Context, request *v2.ConnectorServiceValidateRequest) (*v2.ConnectorServiceValidateResponse, error) { ctx, span := tracer.Start(ctx, "builderImpl.Validate") defer span.End() + start := b.nowFunc() + tt := tasks.ValidateType l := ctxzap.Extract(ctx) retryer := retry.NewRetryer(ctx, retry.RetryConfig{ MaxAttempts: 0, // 0 means no limit - retry indefinitely InitialDelay: 1 * time.Second, MaxDelay: 0, }) for { annos, err := b.cb.Validate(ctx) if err == nil { l.Debug("validation successful") + b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) return &v2.ConnectorServiceValidateResponse{Annotations: annos}, nil } if retryer.ShouldWaitAndRetry(ctx, err) { l.Debug("retrying validation", zap.Error(err), zap.String("status_code", status.Code(err).String())) continue } l.Error("error: validate failed", zap.Error(err), zap.String("status_code", status.Code(err).String())) + b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) return nil, fmt.Errorf("error: validate failed: %w", err) } }
♻️ Duplicate comments (1)
pkg/connectorbuilder/connectorbuilder.go (1)
1000-1001: Remove "error:" prefix from error message for consistency.Based on past review feedback, the "error:" prefix should be removed from error messages as the error will be logged by the caller.
Apply this diff:
- l.Error("error: validate failed", zap.Error(err), zap.String("status_code", status.Code(err).String())) - return nil, fmt.Errorf("error: validate failed: %w", err) + l.Error("validate failed", zap.Error(err), zap.String("status_code", status.Code(err).String())) + return nil, fmt.Errorf("validate failed: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connectorbuilder/connectorbuilder.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/connectorbuilder/connectorbuilder.go (4)
Learnt from: MarcusGoldschmidt
PR: #393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The connectorbuilder.NewConnector function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Learnt from: MarcusGoldschmidt
PR: #393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The connectorbuilder.NewConnector function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Learnt from: ggreer
PR: #194
File: pkg/uhttp/gocache.go:125-132
Timestamp: 2024-07-30T19:21:37.891Z
Learning: Errors when clearing the cache in the GoCache struct are logged later in the process, so additional logging in the Clear method is unnecessary and would cause log spam.
Learnt from: ggreer
PR: #194
File: pkg/uhttp/gocache.go:125-132
Timestamp: 2024-10-08T21:29:30.695Z
Learning: Errors when clearing the cache in the GoCache struct are logged later in the process, so additional logging in the Clear method is unnecessary and would cause log spam.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: go-test (1.23.x, ubuntu-latest)
- GitHub Check: go-test (1.23.x, windows-latest)
| retryer := retry.NewRetryer(ctx, retry.RetryConfig{ | ||
| MaxAttempts: 0, // 0 means no limit - retry indefinitely | ||
| InitialDelay: 1 * time.Second, | ||
| MaxDelay: 0, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider infinite retry configuration.
Setting MaxAttempts: 0 creates infinite retries, which could cause the validation to hang indefinitely if there's a persistent configuration issue. This differs from the Grant and Revoke methods which use MaxAttempts: 3.
Consider using a reasonable maximum attempt limit:
retryer := retry.NewRetryer(ctx, retry.RetryConfig{
- MaxAttempts: 0, // 0 means no limit - retry indefinitely
+ MaxAttempts: 3,
InitialDelay: 1 * time.Second,
- MaxDelay: 0,
+ MaxDelay: 60 * time.Second,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| retryer := retry.NewRetryer(ctx, retry.RetryConfig{ | |
| MaxAttempts: 0, // 0 means no limit - retry indefinitely | |
| InitialDelay: 1 * time.Second, | |
| MaxDelay: 0, | |
| }) | |
| retryer := retry.NewRetryer(ctx, retry.RetryConfig{ | |
| MaxAttempts: 3, | |
| InitialDelay: 1 * time.Second, | |
| MaxDelay: 60 * time.Second, | |
| }) |
🤖 Prompt for AI Agents
In pkg/connectorbuilder/connectorbuilder.go around lines 982 to 986, the retry
configuration uses MaxAttempts: 0, causing infinite retries that may hang
validation indefinitely. Change MaxAttempts to a finite number, such as 3, to
limit retry attempts similarly to the Grant and Revoke methods, preventing
potential endless loops while still allowing retries.
This PR adds retry logic to the
Validatefunction.Summary by CodeRabbit