-
Notifications
You must be signed in to change notification settings - Fork 2
Failover #39
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
Failover #39
Conversation
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.
Pull Request Overview
This PR adds failover and replica discovery support to the Azure App Configuration client.
- Introduces a
ReplicaDiscoveryEnabled
option inOptions
- Defines failover constants and client backoff/jitter logic
- Implements dynamic replica discovery in
configurationClientManager
and a sharedexecuteFailoverPolicy
- Updates load/refresh paths to use the new failover policy and adds comprehensive unit tests
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
azureappconfiguration/options.go | Added ReplicaDiscoveryEnabled flag to configuration |
azureappconfiguration/constants.go | Defined constants for failover intervals and jitter |
azureappconfiguration/client_manager.go | Implemented dynamic replica discovery and backoff logic |
azureappconfiguration/azureappconfiguration.go | Added executeFailoverPolicy , updated load/refresh flows |
azureappconfiguration/failover_test.go | New unit tests for failover behavior and backoff |
Comments suppressed due to low confidence (4)
azureappconfiguration/failover_test.go:1
- [nitpick] Tests cover the failover policy and backoff logic, but there is no coverage for dynamic replica discovery (
getClients
,refreshClients
,discoverFallbackClients
) or endpoint validation (getValidDomain
,isValidEndpoint
). Consider adding unit tests for those paths.
// Copyright (c) Microsoft Corporation.
azureappconfiguration/azureappconfiguration.go:21
- The "maps" import appears unused; remove it to avoid lint or compile warnings.
"maps"
azureappconfiguration/azureappconfiguration.go:595
- After logging, returning the raw error loses context for the caller. Consider wrapping it with fmt.Errorf to preserve context, e.g.,
return false, fmt.Errorf("failed to check feature flag changes: %w", err)
.
log.Printf("Failed to check if feature flag settings have changed: %s", err.Error())
azureappconfiguration/azureappconfiguration.go:613
- Logging and then returning the raw error strips the original context. Consider returning a wrapped error to retain the
"failed to reload feature flag configuration"
prefix.
log.Printf("Failed to reload feature flag configuration: %s", err.Error())
Bump up version 1.1.0
Merge release/v1.1.0 to main 08.01
47a3924
to
6c54d3c
Compare
No description provided.