-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Encryption Api for Bulk Operations #26672
Encryption Api for Bulk Operations #26672
Conversation
…011/azure-sdk-for-java into users/akataria/EncryptionBulkApi
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Please do manual testing as well with to see if this holds good on high number of documents eg 5-10k
...mos-encryption/src/main/java/com/azure/cosmos/encryption/CosmosEncryptionAsyncContainer.java
Outdated
Show resolved
Hide resolved
...mos-encryption/src/main/java/com/azure/cosmos/encryption/CosmosEncryptionAsyncContainer.java
Outdated
Show resolved
Hide resolved
...-cosmos-encryption/src/test/java/com/azure/cosmos/encryption/EncryptionAsyncApiCrudTest.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/batch/BulkExecutor.java
Outdated
Show resolved
Hide resolved
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Please change the title of PR to some description , currently it had branch name in it
...mos-encryption/src/main/java/com/azure/cosmos/encryption/CosmosEncryptionAsyncContainer.java
Outdated
Show resolved
Hide resolved
...mos-encryption/src/main/java/com/azure/cosmos/encryption/CosmosEncryptionAsyncContainer.java
Outdated
Show resolved
Hide resolved
Once all the current comments are resolved and emulator ci run clean, please run the full ci by commenting "/azp run java - cosmos - tests" in the pr. As we have touched the files in Cosmos project , its safe to do that |
return cosmosItemOperationMono; | ||
}); | ||
|
||
final CosmosBulkExecutionOptions cosmosBulkExecutionOptions = bulkOptions; |
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.
This will not work, because encryptionProcessor is not initialized if the encryption does not happen. Encryption happens only when the above flux gets executed. And this header code will get executed before encryption is happening.
This header needs to be set in the flux chain. Please revert this code to its original state.
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.
Reverted the code.
...mos-encryption/src/main/java/com/azure/cosmos/encryption/CosmosEncryptionAsyncContainer.java
Outdated
Show resolved
Hide resolved
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
LGTM , thanks @aayush3011
Ericchoi/main/event hub/2024 01 01 (Azure#26672) * Adding new version 2024-01-01 with changes in Clusters.json
Encryption Api for Bulk Operations