-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Sync-Wrapper for V3 Async SDK #4757
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
Conversation
Contains synchronous operations for - Database - Container - Item operations Scripts, Users, Conflicts etc will be part of subsequent PR
|
/azp run java - cosmos - tests |
|
Pull request contains merge conflicts. |
# Conflicts: # sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/CosmosClientBuilder.java # sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/rx/TestSuiteBase.java
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncClient.java
Show resolved
Hide resolved
sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/rx/TestSuiteBase.java
Show resolved
Hide resolved
sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/sync/CosmosSyncContainerTest.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/sync/CosmosSyncDatabaseTest.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/sync/CosmosSyncItemTest.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/sync/CosmosSyncItemTest.java
Outdated
Show resolved
Hide resolved
moderakh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job Bhaskar I added some minor comments.
sdk/cosmos/sdk/src/test/java/com/azure/data/cosmos/sync/CosmosSyncItemTest.java
Show resolved
Hide resolved
kushagraThapar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, please implement them.
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/CosmosClientBuilder.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncClient.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncClient.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncDatabaseResponse.java
Show resolved
Hide resolved
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncItem.java
Show resolved
Hide resolved
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncItemResponse.java
Show resolved
Hide resolved
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/sync/CosmosSyncContainer.java
Outdated
Show resolved
Hide resolved
Implementing PR comments
sdk/cosmos/sdk/src/main/java/com/azure/data/cosmos/CosmosClientBuilder.java
Outdated
Show resolved
Hide resolved
| * Provides a client-side logical representation of the Azure Cosmos database service. | ||
| * SyncClient is used to perform operations in a synchronous way | ||
| */ | ||
| public class CosmosSyncClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public class CosmosSyncClient { | |
| public final class CosmosSyncClient { |
Based on guidelines, this should be final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we make this final, then the neither the end user nor us can mock this for testing.
| * Instantiate the cosmos client builder to build cosmos client | ||
| * @return {@link CosmosClientBuilder} | ||
| */ | ||
| public static CosmosClientBuilder builder(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(note for ADP folks: This doesn't match the guidelines, but it matches our existing pattern which we've gotten an exception for.)
Contains synchronous operations for
Scripts, Users, Conflicts etc will be part of subsequent PR