Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions sdk/cosmosdb/cosmos/review/cosmos.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ export const Constants: {
IsBatchRequest: string;
IsBatchAtomic: string;
BatchContinueOnError: string;
DedicatedGatewayPerRequestCacheStaleness: string;
ForceRefresh: string;
};
WritableLocations: string;
Expand Down Expand Up @@ -671,6 +672,11 @@ export enum DataType {
String = "String"
}

// @beta
export interface DedicatedGatewayRequestOptions {
maxIntegratedCacheStalenessInMs?: number;
}

// @public (undocumented)
export const DEFAULT_PARTITION_KEY_PATH: "/_partitionKey";

Expand Down Expand Up @@ -730,8 +736,10 @@ export type ExistingKeyOperation = {
// @public (undocumented)
export function extractPartitionKey(document: unknown, partitionKeyDefinition: PartitionKeyDefinition): PartitionKey[];

// Warning: (ae-incompatible-release-tags) The symbol "FeedOptions" is marked as @public, but its signature references "DedicatedGatewayRequestOptions" which is marked as @beta
//
// @public
export interface FeedOptions extends SharedOptions {
export interface FeedOptions extends SharedOptions, DedicatedGatewayRequestOptions {
accessCondition?: {
type: string;
condition: string;
Expand Down Expand Up @@ -1460,8 +1468,10 @@ interface RequestInfo_2 {
}
export { RequestInfo_2 as RequestInfo }

// Warning: (ae-incompatible-release-tags) The symbol "RequestOptions" is marked as @public, but its signature references "DedicatedGatewayRequestOptions" which is marked as @beta
//
// @public
export interface RequestOptions extends SharedOptions {
export interface RequestOptions extends SharedOptions, DedicatedGatewayRequestOptions {
accessCondition?: {
type: string;
condition: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/**
Expand Down
3 changes: 3 additions & 0 deletions sdk/cosmosdb/cosmos/src/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ export const Constants = {
IsBatchAtomic: "x-ms-cosmos-batch-atomic",
BatchContinueOnError: "x-ms-cosmos-batch-continue-on-error",

// Dedicated Gateway Headers
DedicatedGatewayPerRequestCacheStaleness: "x-ms-dedicatedgateway-max-age",

// Cache Refresh header
ForceRefresh: "x-ms-force-refresh",
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/**
* @beta Dedicated Gateway Request Options *Private feature*
*/
export interface DedicatedGatewayRequestOptions {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Blocking]
What's the beta equivalent of Node?
This feature is still in beta, both .NET and JAVA are shipping with beta versions (Preview, @beta).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't really have a story for "beta APIs" - we only have beta versions of packages. So when we have a new major/minor that is not finalized, we will ship beta.1 ... beta.n until it stabilizes and then GA .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will follow up offline.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@xirzec - this approach won't work for Cosmos DB SDK. Please see our other SDKs as well (Java, python) - which also sit in azure sdk mono repo.
Reason this won't work for us is because this DedicatedGateway feature can be in beta phase for 1 or 2 years. We cannot keep shipping beta packages for so long, as customers don't take beta packages in production, and also, we don't fully support beta packages for production use cases.

We need something which is explicit for a feature marked as Beta, while the whole library gets shipped as GA library.
Please see this reference issue - Azure/azure-sdk#2593

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@xirzec - this approach won't work for Cosmos DB SDK. Please see our other SDKs as well (Java, python) - which also sit in azure sdk mono repo.
Reason this won't work for us is because this DedicatedGateway feature can be in beta phase for 1 or 2 years. We cannot keep shipping beta packages for so long, as customers don't take beta packages in production, and also, we don't fully support beta packages for production use cases.

We need something which is explicit for a feature marked as Beta, while the whole library gets shipped as GA library.
Please see this reference issue - Azure/azure-sdk#2593

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@xirzec - first 2 policies make sense to me, however I don't quite agree with 3rd policy.
Thinking about a scenario where we will need to change the API 2-3 times (which has happened in the past in Java SDK at least), that would result in 2-3 new Cosmos Db Node JS SDK versions, which will then create so much chaos.

One hybrid approach that we settled on with Azure arch board regarding beta APIs was that if there are breaking changes in the Beta APIs, we introduce new APIs, and we mark the existing APIs deprecated, keep them in next 2 minor versions, giving customers a chance to fix their code and upgrade.
Would this work on NodeJS SDK?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The guarantee we are trying to give our customers is that if they depend on an @Azure package (e.g. "@azure/cosmos": "^3.15.1" inside their package.json dependencies) we will never break a TS build without doing a major version bump. There are some cases where we change behavior significantly enough that we do a minor version bump, but we'd still want customers who use ~3.15.1 as their version specifier to get patch level bugfixes without worry.

I know other projects (notably TypeScript itself) are a bit more lax around what is a minor versus major change, but really we don't want folks to have to start pinning versions or individually vetting each release as this will delay critical bugfixes and security fixes from being readily adopted.

@bterlson do you have any suggestions about how Cosmos might mitigate risk here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please follow up with me offline. Removing the blocker. I add a comment for experimental.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright I am back with new guidance that I think will work for your scenario. Basically, we'll be aligning with how Office handles this:

  1. Experimental APIs must be tagged with @beta so api-extractor can identify them
  2. After experimental APIs have been marked as @deprecated for two releases they may be removed without a major bump, but there will be a minor version bump.

I believe the second point aligns with @kushagraThapar 's proposal above? @jay-most how do you feel about this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@xirzec - this is great, and aligns with our model of Beta APIs in Java SDK as well! Thank you for the guidance.
@jay-most - can you please make this change?

/**
* Sets the staleness value associated with the request in the Azure CosmosDB service. For requests where the {@link
* com.azure.cosmos.ConsistencyLevel} is {@link com.azure.cosmos.ConsistencyLevel#EVENTUAL}, responses from the
* integrated cache are guaranteed to be no staler than value indicated by this maxIntegratedCacheStaleness. When the
* consistency level is not set, this property is ignored.
*
* <p>Default value is null</p>
*
* <p>Cache Staleness is supported in milliseconds granularity. Anything smaller than milliseconds will be ignored.</p>
*/
maxIntegratedCacheStalenessInMs?: number;
}
3 changes: 2 additions & 1 deletion sdk/cosmosdb/cosmos/src/request/FeedOptions.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
import { DedicatedGatewayRequestOptions } from "..";
import { SharedOptions } from "./SharedOptions";

/**
* The feed options and query methods.
*/
export interface FeedOptions extends SharedOptions {
export interface FeedOptions extends SharedOptions, DedicatedGatewayRequestOptions {
/** Opaque token for continuing the enumeration. Default: undefined
* @deprecated Use continuationToken instead.
*/
Expand Down
4 changes: 2 additions & 2 deletions sdk/cosmosdb/cosmos/src/request/RequestOptions.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
import { SharedOptions } from "./SharedOptions";
import { DedicatedGatewayRequestOptions, SharedOptions } from "..";

/**
* Options that can be specified for a requested issued to the Azure Cosmos DB servers.=
*/
export interface RequestOptions extends SharedOptions {
export interface RequestOptions extends SharedOptions, DedicatedGatewayRequestOptions {
/** Conditions Associated with the request. */
accessCondition?: {
/** Conditional HTTP method header type (IfMatch or IfNoneMatch). */
Expand Down
1 change: 1 addition & 0 deletions sdk/cosmosdb/cosmos/src/request/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ export { SharedOptions } from "./SharedOptions";
export { StatusCode, SubStatusCode } from "./StatusCodes";
export { FeedResponse } from "./FeedResponse";
export { RequestContext } from "./RequestContext";
export { DedicatedGatewayRequestOptions } from "./DedicatedGatewayRequestOptions";
6 changes: 6 additions & 0 deletions sdk/cosmosdb/cosmos/src/request/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ export async function getHeaders({
headers[Constants.HttpHeaders.ConsistencyLevel] = options.consistencyLevel;
}

if (options.maxIntegratedCacheStalenessInMs && resourceType === ResourceType.item) {
headers[Constants.HttpHeaders.DedicatedGatewayPerRequestCacheStaleness] =
options.maxIntegratedCacheStalenessInMs;
}

if (options.resourceTokenExpirySeconds) {
headers[Constants.HttpHeaders.ResourceTokenExpiry] = options.resourceTokenExpirySeconds;
}
Expand Down Expand Up @@ -187,6 +192,7 @@ export async function getHeaders({
if (options.disableRUPerMinuteUsage) {
headers[Constants.HttpHeaders.DisableRUPerMinuteUsage] = true;
}

if (
clientOptions.key ||
clientOptions.resourceTokens ||
Expand Down
90 changes: 90 additions & 0 deletions sdk/cosmosdb/cosmos/test/internal/session.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,96 @@ describe("New session token", function () {
});
});

describe("Integrated Cache Staleness", async function (this: Suite) {
beforeEach(async function () {
await removeAllDatabases();
});
const dbId = addEntropy("maxIntegratedCacheTestDB");
const containerId = addEntropy("maxIntegratedCacheTestContainer");
const dedicatedGatewayMaxAge = 600000;
const client = new CosmosClient({
endpoint,
key: masterKey,
consistencyLevel: ConsistencyLevel.Eventual,
plugins: [
{
on: "request",
plugin: async (context, next) => {
it("Should check if the max integrated cache staleness header is set and the value is correct.", async function () {
if (context.headers["x-ms-consistency-level"]) {
if (context.resourceType === ResourceType.item) {
if (context.headers["x-ms-dedicatedgateway-max-age"]) {
assert.strictEqual(
context.headers["x-ms-dedicatedgateway-max-age"].valueOf(),
dedicatedGatewayMaxAge
);
} else {
assert(
context.headers["x-ms-dedicatedgateway-max-age"],
"x-ms-dedicatedgateway-max-age is not set."
);
assert.ifError(context.headers["x-ms-dedicatedgateway-max-age"]);
}
} else {
assert(
context.headers["x-ms-dedicatedgateway-max-age"],
"Attempt to use x-ms-dedicatedgateway-max-age on a non-item request."
);
assert.ifError(context.headers["x-ms-dedicatedgateway-max-age"]);
}
} else {
assert(
context.headers["x-ms-consistency-level"],
"x-ms-consistency-level is not set."
);
assert.ifError(context.headers["x-ms-consistency-level"]);
}
});
const response = await next(context);
return response;
},
},
],
});
const itemRequestFeedOptions = {
maxIntegratedCacheStalenessInMs: dedicatedGatewayMaxAge,
};
const { database } = await client.databases.createIfNotExists({
id: dbId,
});
const { container } = await database.containers.createIfNotExists({
id: containerId,
});

// Should pass with maxIntegratedCacheStalenessInMs and consistency level set.
await container.items.create({ id: "1" });
await container.item("1").read(itemRequestFeedOptions);
Comment on lines +134 to +136
Copy link
Copy Markdown
Member

@joheredi joheredi Apr 21, 2022

Choose a reason for hiding this comment

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

Would it be possible to split this test into different it() blocks? Even if you have to define a new client with a different plugin, I think it will help test readability and debugging.

Something like this:

it("readItem should pass with maxIntegratedCacheStalenessInMs and consistency level set", async () => {
 const client = new CosmosClient({
    endpoint,
    key: masterKey,
    consistencyLevel: ConsistencyLevel.Eventual,
    plugins: [
      {
        on: "request",
        plugin: async (context, next) => {/** Only check for the specific conditions to pass with maxIntegratedCacheStalenessInMs and consistency level set*/}
      }]
   });
  await container.items.create({ id: "1" });
  await container.item("1").read(itemRequestFeedOptions);
});

it("redDocument should pass with maxIntegratedCacheStalenessInMs and consistency level set. ", async () => {
const client = new CosmosClient({
    endpoint,
    key: masterKey,
    consistencyLevel: ConsistencyLevel.Eventual,
    plugins: [
      {
        on: "request",
        plugin: async (context, next) => {/** Only check for the specific conditions to pass with maxIntegratedCacheStalenessInMs and consistency level set*/}
      }]
   });

  await container.items.readAll(itemRequestFeedOptions).fetchAll();
})

// etc...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jay-most I'd like to hear what you think about the comment about splitting into individual its. The usual pattern for tests is

describe("Test", () => {
   it("should do something", () => {});
   it("should do something else", () => {});
})

However, the current test is executing the it within the constructor plugins which may hurt readability and debugging. I'd suggest inverting it as suggested in my comment above


// Should pass with maxIntegratedCacheStalenessInMs and consistency level set.
// read document.
await container.items.readAll(itemRequestFeedOptions).fetchAll();

// Should pass with maxIntegratedCacheStalenessInMs and consistency level set.
// query documents
const querySpec = {
query: "SELECT * FROM root r WHERE r.id=@id",
parameters: [
{
name: "@id",
value: "1",
},
],
};
await container.items.query(querySpec, itemRequestFeedOptions).fetchAll();

// Should fail: maxIntegratedCacheStalenessInMs should only be set at the item request level and query feed options
assert.doesNotThrow(async () => {
await container.read({
maxIntegratedCacheStalenessInMs: dedicatedGatewayMaxAge,
});
}, "maxIntegratedCacheStalenessInMs should only be set at the item request level and query feed options");
});

// For some reason this test does not pass against the emulator. Skipping it for now
describe.skip("Session Token", function (this: Suite) {
beforeEach(async function () {
Expand Down
1 change: 1 addition & 0 deletions sdk/cosmosdb/cosmos/tsconfig.strict.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
"src/client/Database/index.ts",
"src/documents/index.ts",
"src/request/index.ts",
"src/request/DedicatedGatewayRequestOptions.ts",
"src/index.ts",
"src/retry/index.ts",
"src/queryExecutionContext/Aggregators/index.ts",
Expand Down