Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements startup retry functionality for the Azure App Configuration Go provider, adding resilience during initial configuration loading. The implementation includes exponential backoff with timeout support and comprehensive error handling.
- Adds
StartupOptionsconfiguration with timeout support for initial data loading - Implements retry logic with exponential backoff for startup operations
- Refactors backoff calculation to be reusable across different retry scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| azureappconfiguration/options.go | Adds StartupOptions struct with timeout configuration |
| azureappconfiguration/constants.go | Defines default startup timeout constant (100 seconds) |
| azureappconfiguration/azureappconfiguration.go | Implements startupWithRetry method with timeout and backoff logic |
| azureappconfiguration/client_manager.go | Refactors backoff calculation and adds fixed backoff duration logic |
| azureappconfiguration/failover_test.go | Adds comprehensive test coverage for startup retry scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // StartupOptions is used when initially loading data into the configuration provider. | ||
| type StartupOptions struct { | ||
| // Timeout specifies the amount of time allowed to load data from Azure App Configuration on startup. | ||
| Timeout time.Duration | ||
| } |
There was a problem hiding this comment.
The StartupOptions struct needs an import statement for the time package to be valid.
| timeElapsed := time.Since(startTime) | ||
| backoffDuration := getFixedBackoffDuration(timeElapsed) | ||
| if backoffDuration == 0 { | ||
| backoffDuration = calculateBackoffDuration(attempt) | ||
| } |
There was a problem hiding this comment.
The logic for choosing between fixed and calculated backoff duration is unclear. Consider adding a comment explaining when and why fixed backoff is preferred over exponential backoff.
| // Check if we have enough time left to wait and retry | ||
| timeRemaining := timeout - timeElapsed | ||
| if timeRemaining <= backoffDuration { | ||
| return fmt.Errorf("startup failed after %d attempts within timeout %v: %w", attempt, timeout, err) | ||
| } |
There was a problem hiding this comment.
Maybe I'm missing something, since we have the line 730, does it mean we don't need to have this logic?
There was a problem hiding this comment.
Check at line 722 prevents unnecessary waiting when we know there isn't enough time left for a meaningful retry attempt.
There was a problem hiding this comment.
I see, then let's keep it to avoid unnecessary waiting. But essentially, the error returns in line718 and 724 are the same error, are they? How about unifying the error information? Becuase I think the error message in line 724 is not so friendly to the end user, the startup context is a concept created by us in the code, I think we just need to say load key-values from app config timeout, how do you think?
There was a problem hiding this comment.
sounds reasonable, updated
linglingye001
force-pushed
the
linglingye/startup/retry
branch
from
5d6ee48 to
025fff8
Compare
2 participants
.net provider ref Azure/AppConfiguration-DotnetProvider#488
js ref Azure/AppConfiguration-JavaScriptProvider#166