Skip to content

Support DedicatedGatewayRequestOptions and MaxIntegratedCacheStaleness#21240

Merged
jay-most merged 2 commits intomainfrom
jay/cosmo-dedicated-gateway-request-options
May 12, 2022
Merged

Support DedicatedGatewayRequestOptions and MaxIntegratedCacheStaleness#21240
jay-most merged 2 commits intomainfrom
jay/cosmo-dedicated-gateway-request-options

Conversation

@jay-most
Copy link
Copy Markdown
Contributor

@jay-most jay-most commented Apr 6, 2022

API Surface Change:

  • [ x ] DedicatedGatewayRequestOptions
  • [ x ] FeedtOptions

Implementing max integrated cache staleness requires request consistency to session or eventual. If not, the request will always bypass the integrated cache. The easiest way to configure a specific consistency for all read operations is to set it at the account level. You can also configure consistency at the request level, recommended if you only want a subset of your reads to utilize the integrated cache.

Override the default consistency level (Account level)

const client = new CosmosClient({
      endpoint,
      key: masterKey,
      consistencyLevel: ConsistencyLevel.Eventual,
    });`

Override the default max integrated cache in milliseconds (Item Request level)

    const itemRequestFeedOptions = {
    maxIntegratedCacheStalenessInMs: <maxIntegratedCacheStalenessInMs>, };
  await container.items.create({ id: "1" });

// read documents
  await container.item("1").read(requestOptions);
  await container.items.readAll(requestOptions).fetchAll();

  // query documents
  const querySpec = {
    query: "SELECT * FROM root r WHERE r.id=@id",
    parameters: [
      {
        name: "@id",
        value: "1",
      },
    ],
  };
  await container.items.query(querySpec, requestOptions).fetchAll();

@jay-most jay-most self-assigned this Apr 6, 2022
@ghost ghost added the Cosmos label Apr 6, 2022
@jay-most jay-most force-pushed the jay/cosmo-dedicated-gateway-request-options branch 2 times, most recently from 640adbd to 22dc992 Compare April 7, 2022 18:39
@jay-most jay-most marked this pull request as ready for review April 7, 2022 18:44
@jay-most jay-most force-pushed the jay/cosmo-dedicated-gateway-request-options branch 2 times, most recently from 506078e to a0aeabe Compare April 8, 2022 01:21
@jay-most jay-most force-pushed the jay/cosmo-dedicated-gateway-request-options branch 2 times, most recently from f3865f9 to cb3b3db Compare April 13, 2022 01:32
@jay-most jay-most force-pushed the jay/cosmo-dedicated-gateway-request-options branch 3 times, most recently from 8599e5b to 27ebc14 Compare April 14, 2022 17:31
Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Public surface changes look pretty good. A few minor comments.

@joheredi could you take a quick look as well?

Copy link
Copy Markdown
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Changes look good overall. Left a couple of comments

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?

@jay-most jay-most force-pushed the jay/cosmo-dedicated-gateway-request-options branch from 27ebc14 to 19a043f Compare April 17, 2022 17:32
@jay-most jay-most force-pushed the jay/cosmo-dedicated-gateway-request-options branch 2 times, most recently from 1d4b951 to 6ba17c4 Compare April 21, 2022 04:56
Comment on lines +138 to +136
// Should pass with maxIntegratedCacheStalenessInMs and consistency level set.
await container.items.create({ id: "1" });
await container.item("1").read(itemRequestFeedOptions);
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

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?

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support DedicatedGatewayRequestOptions and MaxIntegratedCacheStaleness

9 participants