-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
App Configuration Spring - V2 Feature Management Schema #40215
base: feature/spring-boot-3
Are you sure you want to change the base?
App Configuration Spring - V2 Feature Management Schema #40215
Conversation
API change check API changes are not detected in this pull request. |
...ud/appconfiguration/config/implementation/feature/entity/FeatureFilterEvaluationContext.java
Show resolved
Hide resolved
...ud/appconfiguration/config/implementation/feature/entity/FeatureFilterEvaluationContext.java
Outdated
Show resolved
Hide resolved
Do we need to update the Besides, is this |
@@ -0,0 +1,78 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
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.
Is there any difference between State
and FeatureFlagState
? Is it possible that we make State
as base class and FeatureFlagState extend State
?
https://github.com/Azure/azure-sdk-for-java/blob/feature/spring-boot-3/sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/State.java
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.
Pre this PR there was only one State
. The idea was that State
had the Configuration Setting that was the watch key, which contains the etag info and we would just go through the list to see if any of them changed. This also worked for feature flags.
Feature Flags now refresh by page, so we need to track the etags by the page, which is part of the MatchConditions
, which is inside of a SettingSelector
. If you look at the confusingly named FeatureFlags
object which is new it contains this SettingSelector
. This is how we now have to track changes for all feature flags.
I'm not a huge fan of this setup, there are a number of reasons why I have this up as a draft. This part will should also work when we add page monitoring for ConfigurationSettings, so we may want to update the names to reflect this.
@@ -0,0 +1,168 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
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.
Looks like we're moving the logic from AppConfigurationFeatureManagementPropertySource
to this FeatureFlagClient
, I'm not sure why we need to do this.
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.
FeatureFlagClient
is here to solve the problem of de-duplicating feature flags loaded from multiple sources. They can come from multiple selects, multiple stores, even from Snapshots. We could have used the old property source class somewhat as is if it wasn't for Snapshots.
Though I just did notice an issue, we might have a logic issue when a store fails to load and we retry from a replica. Will look into.
try { | ||
client.listConfigurationSettings(settingSelector).streamByPage().forEach(pagedResponse -> { | ||
checks.add( | ||
new MatchConditions().setIfNoneMatch(pagedResponse.getHeaders().getValue(HttpHeaderName.ETAG))); |
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.
what is this checks
used for? To check if there's a "ETAG" header?
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.
checks
is a list of MatchConditions
. The MatchConditions
class contains the logic from the SDK for paged tags. if you look down a few lines we add the checks
list to the SettingSelector
. This modified SettingSelector
will be the one used in refresh to check for changes.
Description
Updates Spring Provider to the V2 Feature Management schema.
https://github.com/Azure/AppConfiguration/blob/main/docs/FeatureManagement/FeatureManagement.v2.0.0.schema.json
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines