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.

Sign up for GitHub

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

Jump to bottom

Transactional batch for Java #15775

Merged
merged 31 commits into from
Oct 12, 2020
Merged

Transactional batch for Java #15775

merged 31 commits into from
Oct 12, 2020

Conversation

rakkuma
Copy link
Contributor

@rakkuma rakkuma commented Sep 28, 2020

This PR adds Transactional batch feature in Java SDK.

  1. Use TransactionalBatch.createTransactionalBatch(PartitionKey) or new TransactionalBatch(PartitionKey) to create an instance of TransactionalBatch.

  2. Add operations in it like createItem/deleteItem/replaceItem/upsertItem/readItem for same partition key which you want to execute in all or nothing fashion.

  3. Execute it using the container instance like container.executeTransactionalBatch(batch) which will return a TransactionalBatchResponse which will have array of responses and one response per operation

Implementation:

  1. TransactionalBatch keeps the operation and partition key for a batch.
  2. When passed to container for execution, the operations are used to create a ServerBatchRequest instance, which help in serializing the request.
  3. It is executed like a normal request with batch operation type. Actual content is fetched from ServerBatchRequest which have it in a serialized form.
  4. Backend response is parsed in BatchResponseParser to create a array of results i.e. one result per operation. The cumulative result is wrapped in TransactionalBatchResponse.
  5. Response for a particular operation can be fetched using getOperationResultAtIndex which takes index param and the class type of the operation(it will parse the response item too) or simply get(index) which won't parse the item.

@rakkuma
Transactional batch seperate
1ce5af2 
Signed-off-by: Rakesh Kumar <[email protected]>
@ghost ghost added the Cosmos label Sep 28, 2020
moderakh
moderakh suggested changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

I added some specific comments to api and implementation.

The following ones are common comments which are applicable to different classes. Summarizing them here to avoid repeating them everywhere. Please go over these and ensure addressed everywhere.

  1. any class in com.azure.cosmos is in our public apis (Except BridgeInternal). in this package, If a method is not meant to be used by the end user, then it should not be marked as public. If a method in the public package is only meant to be used for internal implementation by other java packages, then it should be package private and you should use BridgeInternal technique to access the method from other packages.
    There are places in different public classes that setter or constructor is only used by implementation. those should not be public.

  2. implementing AutoClosable in batch response. It seems to me it is a 1-1 port from dot-net disposable. Do you have any specific use-case in Java? I didn't see anything in close() method which requires special handing.

  3. sublcassing List. that seems smelly to me why we should do that. other option is to just add the methods which are relevant instead of subclassing List.

  4. please follow our test pattern both for assertion and also re-using shared db/container resources as our tests are running against prod

@FabianMeiswinkel
Copy link
Member

FabianMeiswinkel commented Sep 29, 2020 •
edited
Loading

Nice-to-have (I will pick it up after merging the PR if you don't get to it): I think it would be nice to have a benchmark operation in the .Net benchmark tool (which is available under azure-cosmos-dotnet-benchmark in a Java-ported version now as well). That way we could compare throughput results between .Net and Java to get a baseline. Probably the easiest way to get a feeling for performance on TransactionalBatch in Java.

FabianMeiswinkel
FabianMeiswinkel requested changes Sep 29, 2020
View reviewed changes
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

Overall LGTM - blocking mainly on the question around NPE vs. IllegalArgumentException and types from implementation package leaking into public surface area

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mitchdenny
Copy link
Contributor

/azp run java - cosmos - tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mitchdenny
Copy link
Contributor

/azp run java - cosmos - tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mitchdenny
Copy link
Contributor

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Contributor

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Contributor

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rakkuma
Copy link
Contributor Author

rakkuma commented Oct 9, 2020

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

FabianMeiswinkel
FabianMeiswinkel reviewed Oct 9, 2020
View reviewed changes
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/TransactionalBatch.java Outdated
*
* @return The transactional batch instance with the operation added.
*/
public <T> CosmosItemOperation createItem(T item) {
Copy link
Member

Choose a reason for hiding this comment

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

NIT - as discussed I think naming the factory methods xxxItemOperation here - like createItemOperation or upsertItemOperation would be clearer.

FabianMeiswinkel
FabianMeiswinkel approved these changes Oct 9, 2020
View reviewed changes
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

Thanks Rakesh - LGTM now except for the payload size comment from Mo and the NIT on the naming. Thanks for your due diligence - great work!

@rakkuma
Fixing comment
17ea44b 
Signed-off-by: Rakesh Kumar <[email protected]>
moderakh
moderakh reviewed Oct 9, 2020
View reviewed changes
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/TransactionalBatch.java Outdated
* @return The list of operations which are to be executed.
*/
List<ItemBatchOperation<?>> getOperationsInternal() {
return UnmodifiableList.unmodifiableList(operations);
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate.
you can rely on getOperations() public method.

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 changed it to use CosmosItemOperation everywhere except when serializing it.

@rakkuma
Fix
4c4893d 
Signed-off-by: Rakesh Kumar <[email protected]>
@rakkuma
Copy link
Contributor Author

rakkuma commented Oct 9, 2020

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rakkuma
Reverting byte buffer change
b707495 
Signed-off-by: Rakesh Kumar <[email protected]>
@rakkuma
Copy link
Contributor Author

rakkuma commented Oct 9, 2020

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rakkuma rakkuma merged commit 962f327 into Azure:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j82w j82w j82w left review comments

@moderakh moderakh moderakh approved these changes

@FabianMeiswinkel FabianMeiswinkel FabianMeiswinkel approved these changes

@David-Noble-at-work David-Noble-at-work Awaiting requested review from David-Noble-at-work

@kirankumarkolli kirankumarkolli Awaiting requested review from kirankumarkolli

@kushagraThapar kushagraThapar Awaiting requested review from kushagraThapar

@mbhaskar mbhaskar Awaiting requested review from mbhaskar

@milismsft milismsft Awaiting requested review from milismsft

@simplynaveen20 simplynaveen20 Awaiting requested review from simplynaveen20

@xinlian12 xinlian12 Awaiting requested review from xinlian12

Assignees
No one assigned
Labels
Cosmos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rakkuma @FabianMeiswinkel @moderakh @kushagraThapar @mikeharder @mitchdenny @kirankumarkolli @j82w