Skip to content
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

Add IConfigurationRefresher.TryRefresh API #113

Closed
zhenlan opened this issue Dec 13, 2019 · 8 comments
Closed

Add IConfigurationRefresher.TryRefresh API #113

zhenlan opened this issue Dec 13, 2019 · 8 comments
Assignees
Labels
breaking change enhancement New feature or request

Comments

@zhenlan
Copy link
Contributor

zhenlan commented Dec 13, 2019

To make it easier for customers to trigger configuration refresh without worrying about crashing the app due to various transient errors, propose to add the API below in IConfigurationRefresher.

Task<bool> TryRefresh()

Details
The TryRefresh will not throw on Transient or any FailedRequestException, regardless of the status code (even 401, 403)

The existing API Refresh will not catch anything.

More discussion
Propose to follow the convention and rename APIs to

Task RefreshAsync();
Task<bool> TryRefreshAsync();
@zhenlan
Copy link
Contributor Author

zhenlan commented Dec 13, 2019

cc: @jimmyca15 @drago-draganov

@jimmyca15
Copy link
Member

@zhenlan I'm surprised that we don't want to throw on 401 and 403. Those aren't transient and will never recover during the life time of the application. I can understand a user not wanting TryRefresh to fail because they want to do best effort refreshes and not have transient errors crash things, but now they can't detect the case where it is impossible for subsequent TryRefresh to succeed.

I would suggest that TryRefresh still throws UnauthorizedAccessException in the case of 403 or 401.

@drago-draganov
Copy link
Contributor

@jimmyca15
401 and 403 can be transient for Refresh. Consider AAD or RBAC permissions modified during application being online. Why should the application crash during refresh - it already got config initially? The customer can fix the issue, externally, by setting up the right permissions, without the need to restart the app.

@jimmyca15
Copy link
Member

What I mean by transient is that the error can go away without any action by the user other than retrying. I see arguments for both ways and am fine with either, after all it is based off what the scenario is that we expect users to call TryRefresh.

@drago-draganov
Copy link
Contributor

From API prospective it's not transient, but from use case it could be. I think we are looking on scenario based definition here. App doesn't need to crash on refresh.

@jimmyca15
Copy link
Member

Okay, I agree with that then. If the API intention is a no-crash refresh then it makes sense.

@drago-draganov
Copy link
Contributor

We may need to look at logging in such case, though.

@abhilasharora
Copy link
Contributor

Closing this issue since the proposed APIs RefreshAsync and TryRefreshAsync have been released as a part of version 3.0.0 of the package Microsoft.Extensions.Configuration.AzureAppConfiguration.

The addition of logging to TryRefreshAsync is being tracked at #134.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants