8000 Storage: Fix slow download performance by ajaaym · Pull Request #5791 · googleapis/google-cloud-java · GitHub
[go: up one dir, main page]
Skip to content

Storage: Fix slow download performance #5791

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
Merged
ajaaym merged 8 commits into from
Jul 31, 2019
Merged

Storage: Fix slow download performance #5791

ajaaym merged 8 commits into from
Jul 31, 2019

Conversation

@ajaaym
Copy link
8000
Contributor
@ajaaym ajaaym commented Jul 19, 2019

Fixes #3929 Updated blob to download using direct download method

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 19, 2019
@codecov
Copy link
codecov bot commented Jul 19, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@4dbff6e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5791   +/-   ##
=========================================
  Coverage          ?   46.51%           
  Complexity        ?    25648           
=========================================
  Files             ?     2456           
  Lines             ?   267497           
  Branches          ?    29751           
=========================================
  Hits              ?   124415           
  Misses            ?   133039           
  Partials          ?    10043
Impacted Files Coverage Δ Complexity Δ
...e/src/main/java/com/google/cloud/storage/Blob.java 82.35% <100%> (ø) 30 <1> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dbff6e...cb65519. Read the comment docs.

@ajaaym ajaaym changed the title [WIP]Storage: Fix #3929 Slow download performance for Storage API Storage: Fix #3929 Slow download performance for Storage API Jul 19, 2019
@ajaaym
Copy link
Contributor Author
ajaaym commented Jul 19, 2019

@JesseLovelace @frankyn can you please review?

@frankyn frankyn self-requested a review July 25, 2019 17:08
frankyn
frankyn suggested changes Jul 27, 2019
View reviewed changes
Copy link
Contributor
@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Apologies for the delay, I didn't click submit.

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java Outdated
* @param outputStream
*/
public void downloadTo(OutputStream outputStream) {
downloadTo(outputStream, new BlobSourceOption[0]);
Copy link
Contributor
@frankyn frankyn Jul 25, 2019

Choose a reason for hiding this comment

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

This overloaded method doesn't look necessary. My varargs are rusty but if no Options are specified in downloadTo(OutputStream os, Options..) it should still work.

Copy link
Contributor Author
@ajaaym ajaaym Jul 30, 2019

Choose a reason for hiding this comment

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

yes its not needed. removed it.

google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java
< 8000 td data-line-number="136" class="blob-num blob-num-context">
private static final String BASE64_KEY = "JVzfVl8NLD9FjedFuStegjRfES5ll5zc59CIXw572OA=";
private static final Key KEY =
new SecretKeySpec(BaseEncoding.base64().decode(BASE64_KEY), "AES256");
private static final RetrySettings RETRY_SETTINGS =
Copy link
Contributor
@frankyn frankyn Jul 25, 2019

Choose a reason for hiding this comment

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

There's heavy retry settings in the tests. Is this by design and how do they help?

Copy link
Contributor Author
@ajaaym ajaaym Jul 30, 2019

Choose a reason for hiding this comment

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

updated retry settings needed for test.

frankyn
frankyn suggested changes Jul 30, 2019
View reviewed changes
Copy link
Contributor
@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Thanks @ajaaym, I've have two more nits. Mainly, I'd like to see documentation in comments near the unit tests exercising downloadTo().

google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java Outdated
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.threeten.bp.Duration;
Copy link
Contributor
@frankyn frankyn Jul 30, 2019

Choose a reason for hiding this comment

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

Is this a necessary import. It looks unused.

google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java
RetrySettings.newBuilder()
.setMaxAttempts(2)
.build();
private static final ApiClock API_CLOCK =
8000 Copy link
Contributor
@frankyn frankyn Jul 30, 2019

Choose a reason for hiding this comment

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

Please document how retries are being exercised in these unit tests. It will help for posterity.

@frankyn
Copy link
Contributor
frankyn commented Jul 30, 2019

cc: @jadekler, he wants to use the overload Blob.downloadTo(OutputStream, ...).

@chingor13 chingor13 changed the title Storage: Fix #3929 Slow download performance for Storage API Storage: Fix slow download performance Jul 30, 2019
frankyn
frankyn approved these changes Jul 31, 2019
View reviewed changes
Copy link
Contributor
@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Pending tests passing.

@frankyn
Copy link
Contributor
frankyn commented Jul 31, 2019

Thanks for your patience @ajaaym!

@ajaaym ajaaym merged commit 82298d0 into googleapis:master Jul 31, 2019
@ajaaym ajaaym mentioned this pull request Jul 31, 2019
Merged
@kolea2 kolea2 mentioned this pull request Jul 31, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@frankyn frankyn frankyn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JesseLovelace JesseLovelace

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slow download performance for Storage API

4 participants

@ajaaym @frankyn @googlebot @JesseLovelace
0