-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Cosmos] Simple endpoint refresh interval #15781
Conversation
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 a good start! Left some minor comments.
@@ -236,6 +236,7 @@ export enum ConnectionMode { | |||
export interface ConnectionPolicy { | |||
connectionMode?: ConnectionMode; | |||
enableEndpointDiscovery?: boolean; | |||
endpointRefreshRate?: number; |
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.
Can we suffix this with units, e.g. endpointRefreshRateInMs
?
console.warn(`Failed to refresh endpoints: ${e}`) | ||
} | ||
}, refreshRate) | ||
this.endpointRefresher.unref(); |
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.
does this code run in the browser too? unref
won't work there
} catch (e) { | ||
console.warn(`Failed to refresh endpoints: ${e}`) | ||
} | ||
}, refreshRate) |
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.
nit
}, refreshRate) | |
}, refreshRate); |
try { | ||
globalEndpointManager.refreshEndpointList(); | ||
} catch (e) { | ||
console.warn(`Failed to refresh endpoints: ${e}`) |
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.
not sure if it makes a difference, but I like to pass the exception as its own arg (you sometimes get better inspection that way in the browser at least):
console.warn(`Failed to refresh endpoints: ${e}`) | |
console.warn("Failed to refresh endpoints", e); |
@@ -21,6 +21,8 @@ export interface ConnectionPolicy { | |||
* Default is `false`. | |||
*/ | |||
useMultipleWriteLocations?: boolean; | |||
/** Rate at which the client will refresh the endpoints list in the background */ |
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.
definitely need the units here :)
@@ -93,6 +94,9 @@ export class CosmosClient { | |||
async (opts: RequestOptions) => this.getDatabaseAccount(opts) | |||
); | |||
this.clientContext = new ClientContext(optionsOrConnectionString, globalEndpointManager); | |||
if (optionsOrConnectionString.connectionPolicy?.enableEndpointDiscovery) { |
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.
we should make sure the ref docs for enableEndpointDiscovery
talk about the requirement to call dispose
it("should fetch new endpoints", async function () { | ||
const client = new CosmosClient({ endpoint, key: masterKey }); | ||
const { resource: { writableLocations, readableLocations }} = await client.getDatabaseAccount(); | ||
console.log({ writableLocations, readableLocations }) |
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.
assuming these are in progress 😉
retryOptions: { | ||
maxRetryAttemptCount: 9, | ||
fixedRetryIntervalInMilliseconds: 100, | ||
maxWaitTimeInSeconds: 30 | ||
}, | ||
useMultipleWriteLocations: true, | ||
endpointRefreshRateInMs: 300000 |
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.
This is not a breaking change? Just want to verify
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.
it's a default on the defaultConnectionPolicy
and is optional so it should be fine. Let me verify though
@@ -175,7 +175,7 @@ export class GlobalEndpointManager { | |||
this.writeableLocations.push(location); | |||
} | |||
} | |||
for (const location of databaseAccount.writableLocations) { | |||
for (const location of databaseAccount.readableLocations) { |
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.
@southpolesteve do you think this was a bug? this causes test failures
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.
I am not sure. Did this method work before this change? Let's discuss tomorrow
Adds a simple endpoint refresh interval to replace https://github.com/Azure/azure-sdk-for-js/pull/6283/files#diff-a8b952f4fc200bc79ab6c81a6c4a4cf8c72edcec6042b90245c82ae28d6f51b9L134-L153