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

[AppConfig] Not share SyncTokenPolicy across Configuration clients #33031

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Jan 17, 2023

Description

SyncTokenPolicy should be exist as a single standalone object per Configuration client but not shared cross clients.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. App Configuration Azure.ApplicationModel.Configuration labels Jan 17, 2023
@mssfang mssfang added this to the 2023-02 milestone Jan 17, 2023
@mssfang mssfang requested a review from alzimmermsft as a code owner January 17, 2023 18:09
@mssfang mssfang self-assigned this Jan 17, 2023
@mssfang mssfang requested a review from mrm9084 January 17, 2023 18:10
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@@ -186,7 +185,7 @@ public ConfigurationClientBuilder() {
* and {@link #retryPolicy(HttpPipelinePolicy)} have been set.
*/
public ConfigurationClient buildClient() {
return new ConfigurationClient(buildInnerClient(), DEFAULT_SYNC_TOKEN_POLICY);
return new ConfigurationClient(buildInnerClient(), new SyncTokenPolicy());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the SyncTokenPolicy used in the pipeline and the one passed to the ConigurationClient need to be the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyncTokenPolicy needs to be the same object. Updated.

Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes for SyncTokenPolicy look good

@mssfang mssfang merged commit d654818 into Azure:main Jan 17, 2023
@mssfang mssfang deleted the AppConfig-bug-singleClient branch January 17, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants