-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refresh key value collection based on page etag #133
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhiyuanliang-ms
requested review from
Eskibear,
avanigupta,
juniwang,
rossgrambo and
linglingye001
November 18, 2024 14:47
zhiyuanliang-ms
force-pushed
the
zhiyuanliang/register-all-refresh
branch
from
November 18, 2024 17:12
0b6d9c0
to
fdd30e2
Compare
zhiyuanliang-ms
commented
Nov 18, 2024
zhiyuanliang-ms
changed the title
Key Value refresh based on pageEtag
refresh keyvalue based on page etag
Nov 18, 2024
zhiyuanliang-ms
changed the title
refresh keyvalue based on page etag
Refresh keyvalue based on page etag
Nov 18, 2024
zhiyuanliang-ms
changed the title
Refresh keyvalue based on page etag
Refresh key value collection based on page etag
Nov 18, 2024
…avaScriptProvider into zhiyuanliang/register-all-refresh
Open
LGTM |
Closed
rossgrambo
reviewed
Dec 4, 2024
rossgrambo
reviewed
Dec 4, 2024
linglingye001
approved these changes
Dec 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this PR?
This PR supports refreshing key value based on page etag (aka key value collection monitoring).
Visible change
When refreshOption.enabled is true and no watched setting is specified, then the provider will watch all selected key value to refresh the configuration
This PR also re-organize the
AzureAppConfigurationImpl
(1. adjust the method order 2. add more comments 3. simplify the code path 4. make more function resuableNote
Some corner cases:
with this load option, the
feature_management
section will not appear in the local configuration (provider will ignore feature flag configuration settings), because user doesn't use the feature flag in the load options (i.e.AzureAppConfigurationOptions.feaetureFlagOptions
.However, if users changes any feature flag through the portal, the page etag of monitored key value collection (of selector with keyFilter of "*") will change. So it will trigger a real refresh. Even if, all the key value loaded to local configuration will be the same as the previous one.
This behavior may only affect the scenario when
onRefresh
callback is used. Otherwise, user will never notice this.Reference
.NET provider PR