[Cosmos] Simple endpoint refresh interval #15781
Conversation
ghost
added
the
| @@ -236,6 +236,7 @@ export enum ConnectionMode { | |||
| export interface ConnectionPolicy { | |||
| connectionMode?: ConnectionMode; | |||
| enableEndpointDiscovery?: boolean; | |||
| endpointRefreshRate?: number; | |||
There was a problem hiding this comment.
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.
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.
nit
| }, refreshRate) | |
| }, refreshRate); |
| try { | ||
| globalEndpointManager.refreshEndpointList(); | ||
| } catch (e) { | ||
| console.warn(`Failed to refresh endpoints: ${e}`) |
There was a problem hiding this comment.
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.
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.
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.
assuming these are in progress 😉
| retryOptions: { | ||
| maxRetryAttemptCount: 9, | ||
| fixedRetryIntervalInMilliseconds: 100, | ||
| maxWaitTimeInSeconds: 30 | ||
| }, | ||
| useMultipleWriteLocations: true, | ||
| endpointRefreshRateInMs: 300000 |
There was a problem hiding this comment.
This is not a breaking change? Just want to verify
There was a problem hiding this comment.
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.
@southpolesteve do you think this was a bug? this causes test failures
There was a problem hiding this comment.
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