Skip to content

Conversation

@heguro
Copy link

@heguro heguro commented Feb 18, 2025

Packages impacted by this PR

@azure/cosmos

Issues associated with this PR

Describe the problem that is addressed by this PR

JSDoc comment and documentation incorrectly state that enableBackgroundEndpointRefreshing defaults to false, when the actual default value is true as shown in the defaultConnectionPolicy object:

enableBackgroundEndpointRefreshing: true,

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

#15781 (implemented this parameter)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@github-actions github-actions bot added Community Contribution Community members are working on the issue Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Feb 18, 2025
@github-actions
Copy link

Thank you for your contribution @heguro! We will review the pull request and get back to you soon.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@github-actions
Copy link

Hi @heguro. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@github-actions github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Apr 25, 2025
@amanrao23
Copy link
Member

During client initialization, user has the option to specify connection policy.

If no connection policy is supplied, then defaultConnectionPolicy is picked up (Value of enableBackgroundEndpointRefreshing flag is true here)

If a connection policy is supplied and enableBackgroundEndpointRefreshing is not set in the custom connection policy, then by default it picks up value as false.

@amanrao23 amanrao23 closed this Apr 25, 2025
@heguro
Copy link
Author

heguro commented Apr 25, 2025

If a connection policy is supplied and enableBackgroundEndpointRefreshing is not set in the custom connection policy, then by default it picks up value as false.

I believe this is not correct. My understanding is that it is only treated as false if enableBackgroundEndpointRefreshing: false (or any falsy value) is explicitly set during client initialization.

optionsOrConnectionString.connectionPolicy = Object.assign(
{},
defaultConnectionPolicy,
optionsOrConnectionString.connectionPolicy,
);

As seen here, Object.assign() merges defaultConnectionPolicy with the connectionPolicy provided by the user. Unless the user explicitly sets the enableBackgroundEndpointRefreshing property, the default value of true from defaultConnectionPolicy is applied.

For example, when configured as shown below, enableBackgroundEndpointRefreshing is treated as true, and I confirmed that this.backgroundRefreshEndpointList() is executed within CosmosClient.ts.

const client = new CosmosClient({
  endpoint: "something",
  key: "something",
  connectionPolicy: {
    requestTimeout: 1_000_000,
    // enableBackgroundEndpointRefreshing property is not specified
  },
});

If no connection policy is supplied, then defaultConnectionPolicy is picked up (Value of enableBackgroundEndpointRefreshing flag is true here)

Furthermore, this behavior (that the default is true when no connection policy is supplied or when the property is not explicitly set in a supplied policy) is not mentioned anywhere in the documentation. This lack of documentation, requiring users to read the source code to understand the actual default behavior, can be confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community Contribution Community members are working on the issue Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. no-recent-activity There has been no recent activity on this issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants