-
Notifications
You must be signed in to change notification settings - Fork 275
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
Integrate Azure storage v12 into AzureCloudDestination #1324
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1324 +/- ##
============================================
- Coverage 72.15% 72.12% -0.03%
+ Complexity 6498 6496 -2
============================================
Files 472 472
Lines 37265 37204 -61
Branches 4707 4703 -4
============================================
- Hits 26888 26835 -53
+ Misses 9124 9118 -6
+ Partials 1253 1251 -2
Continue to review full report at Codecov.
|
ambry-cloud/src/main/java/com.github.ambry.cloud/azure/AzureCloudDestination.java
Outdated
Show resolved
Hide resolved
ambry-cloud/src/main/java/com.github.ambry.cloud/azure/AzureBlobDataAccessor.java
Outdated
Show resolved
Hide resolved
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.
Do remember to run AzureIntegrationTest before checking in.
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 after addressing these comments.
ambry-cloud/src/main/java/com.github.ambry.cloud/azure/AzureBlobDataAccessor.java
Outdated
Show resolved
Hide resolved
RequestRetryOptions requestRetryOptions = new RequestRetryOptions(); | ||
storageClient = new BlobServiceClientBuilder().connectionString(azureCloudConfig.azureStorageConnectionString) | ||
.httpClient(client) | ||
.retryOptions(requestRetryOptions) | ||
.configuration(storageConfiguration) |
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.
storageConfiguration
appears to now be empty. does it still have to be passed into the 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.
I originally was setting something in it and removed it. Figured I'd keep it as a placeholder in case we need it later.
ambry-cloud/src/main/java/com.github.ambry.cloud/azure/AzureBlobDataAccessor.java
Outdated
Show resolved
Hide resolved
ambry-cloud/src/main/java/com.github.ambry.cloud/azure/AzureBlobDataAccessor.java
Outdated
Show resolved
Hide resolved
@@ -123,6 +123,7 @@ subprojects { | |||
// TODO audit and fix our javadocs so that we don't need this setting | |||
// This is mainly for cases where param/throws tags don't have descriptions | |||
// Previously, javadocs weren't being compiled, but now shipkit automatically enables this build step | |||
options.addStringOption('Xdoclint:none', '-quiet') // Suppress lint warnings |
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.
thanks for finding this option!
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.
I couldn't find a way to really customize it and error out on specific things, it's basically all or nothing so I chose nothing to cut down the noise.
Also remove unused CloudDestination.doesBlobExist()