Skip to content
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] Bulk/Batch APIs with v1/v2 hashing in JS #10168

Merged
merged 31 commits into from
Jul 28, 2020

Conversation

zfoster
Copy link
Contributor

@zfoster zfoster commented Jul 21, 2020

Adds v1 and v2 partitionKey hashing on the client. See: #9821 for more details as the PR has more thorough notes.

This PR more generally adds support for the bulk/batch API in Cosmos. From a user perspective, this involves passing a list of operations to the container.items.bulk() method. Here's an example of what those operations looks like and the following bulk call: https://github.com/Azure/azure-sdk-for-js/compare/master...zfoster:zf/bulk?expand=1#diff-536cb613fdfa2c9c4361ce96f07b223bR330

Currently, this PR only supports bulk operations on items with partitionKey value types string, number, {}, boolean, or null. To represent undefined, we can use {}. It supports v1 and v2 containers.

@ghost ghost added the Cosmos label Jul 21, 2020
@zfoster zfoster requested a review from xirzec as a code owner July 22, 2020 18:57
@@ -54,6 +54,14 @@ export class ChangeFeedResponse<T> {
export class ClientContext {
constructor(cosmosClientOptions: CosmosClientOptions, globalEndpointManager: GlobalEndpointManager);
// (undocumented)
bulk<T>({ body, path, resourceId, partitionKeyRange, options }: {

Choose a reason for hiding this comment

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

partitionKeyRangeId for consistency

@@ -759,6 +770,11 @@ export class ItemResponse<T extends ItemDefinition> extends ResourceResponse<T &
// @public
export class Items {
constructor(container: Container, clientContext: ClientContext);
// Warning: (ae-forgotten-export) The symbol "Operation" needs to be exported by the entry point index.d.ts

Choose a reason for hiding this comment

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

Please fix.

.map(async (batch: Batch) => {
try {
const response = await this.clientContext.bulk({
body: batch.operations,

Choose a reason for hiding this comment

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

Each service request can take 100 operations and be up to 2 MB (recommend ~200 KB unless a single doc is bigger which which case fit only one operation into the request).

};

request.headers = await this.buildHeaders(request);
request.headers[Constants.HttpHeaders.IsBatchRequest] = "True";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
request.headers[Constants.HttpHeaders.IsBatchRequest] = "True";
request.headers[Constants.HttpHeaders.IsBatchRequest] = true;

sdk/cosmosdb/cosmos/src/client/Item/Items.ts Show resolved Hide resolved
import { doubleToByteArrayJSBI, writeNumberForBinaryEncodingJSBI } from "./encoding/number";
import { writeStringForBinaryEncoding } from "./encoding/string";
import { BytePrefix } from "./encoding/prefix";
// tslint:disable-next-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

@@ -0,0 +1,51 @@
import { doubleToByteArrayJSBI } from "./encoding/number";
import { BytePrefix } from "./encoding/prefix";
// tslint:disable-next-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had requires in there originally. just leftover, i'll remove

@southpolesteve
Copy link
Contributor

@zfoster Can you also add a change log entry?

},
{
operationType: "Upsert",
resourceBody: { id: addEntropy("doc2"), name: "other", key: "A" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resourceBody: { id: addEntropy("doc2"), name: "other", key: "A" }
resourceBody: { id: "doc2", name: "other", key: "A" }

{
operationType: "Read",
id: "readItemId",
partitionKey: `["key"]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we "headerify" the partition key for people? IE, let them supply "key"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we can

@southpolesteve southpolesteve self-requested a review July 27, 2020 23:40
@southpolesteve southpolesteve merged commit 821f350 into Azure:master Jul 28, 2020
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.

3 participants