3929 Slow download performance for Storage API. Added new downloadToPathWithMediaHttpDownloader method with better performance. #4337

andrey-qlogic · 2019-01-14T18:48:03Z

Fixes #3929 Added new downloadToPathWithMediaHttpDownloader method with better performance.

andrey-qlogic · 2019-01-14T18:58:34Z

@frankyn
The approach is tied to an obsolete client, but the performance of the new method downloadToPathWithMediaHttpDownloader
looks better than the other downloadTo method . I would suggest adding this approach to the user separately from another method. If it looks reasonable, then I will add also unit tests.

sduskis · 2019-01-15T00:01:19Z

@andrey-qlogic, can you please add a descriptive title?

mcantrell · 2019-01-15T21:48:41Z

I would suggest creating a method that is decoupled from java.nio.file.Path. I would assume that a lot of users (myself included) would not have a file to write to. For example, you may want to stream the data from the storage API to a browser via HTTP response

    downloadWithMediaHttpDownloaderTo(OutputStream out)

and/or

    downloadWithMediaHttpDownloaderTo(WritableChannel out)

andrey-qlogic · 2019-01-16T18:22:04Z

@mcantrell
Changing to downloadWithMediaHttpDownloaderTo(OutputStream out, ...) is more appreciate way than using WritableChannel.
But the definition of the next parameter becomes complicated for User we have either build com.google.api.services.storage.Storage with HttpStorageOptions that may not available in StorageOptions for getting MediaHttpDownloader.We have to make the second parameter com.google.api.services.storage.Storage and make the user responsible for building it the right way. I would appreciate your input here.

public void downloadToPathWithMediaHttpDownloader(Path path, com.google.api.services.storage.Storage storage)

mcantrell · 2019-01-17T15:42:27Z

Sorry, I don't know the internals in detail but wouldn't the blob's storage object contain the transport options? Building the client like this?

  public void downloadWithMediaHttpDownloader(OutputStream targetOut) {
    com.google.api.services.storage.Storage storage = buildStorage(this.storage.getOptions());

mcantrell · 2019-01-17T15:42:39Z

I suppose that another alternative to using the media downloader is to do something similar to the BlobReaderChannel. You can use this.storage.getOptions().getStorageRpcV1().read() to fetch the required bytes using the CountingOutputStream's offset and bytes remaining.

edit: never mind, that won't work. it returns a byte array. Not sure what I was thinking here. Maybe I hadn't had enough coffee yet :)

andrey-qlogic · 2019-01-18T16:21:11Z

Sorry, I don't know the internals in detail but wouldn't the blob's storage object contain the transport options? Building the client like this?
  public void downloadWithMediaHttpDownloader(OutputStream targetOut) {
    com.google.api.services.storage.Storage storage = buildStorage(this.storage.getOptions());

the change to OutputStream is reasonable, but building com.google.api.services.storage.Storage object from Blob may not be a good idea.

frankyn · 2019-01-18T22:51:43Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

+    } catch (IOException e) {
+      throw new StorageException(e);
+    }
+  }


@andrey-qlogic, I appreciate your patience.

I'd recommend passing through an additional option (USE_DIRECT_DOWNLOAD) to downloadTo() if using getMediaHttpDownloader is considered a breaking change:

Then in the underlying RPC class handle the request in
HttpStorageRpc.getCall() which called by HttpStorageRpc.get().

WDYT?

Added USE_DIRECT_DOWNLOAD to set directDownloadEnabled for MediaHttpDownloader

frankyn

Thanks @andrey-qlogic, I left a few comments. Also have you benchmarked this change to compare?

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

andrey-qlogic · 2019-01-24T20:50:14Z

I still need more benchmarking

andrey-qlogic · 2019-01-25T16:25:44Z

@frankyn, it turned out that adding the new option USE_DIRECT_DOWNLOAD and setting MediaHttpDownloader directDownloadEnable to true/false depends on the option does not change the performance. The throughput still ~10Mb/s when the legacy client showed 40Mb/s. At the same time, if I increase buffer allocating size 100x times at the 'downloadTo' method

google-cloud-java/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

Line 217 in 9c47e1b

ByteBuffer bytes = ByteBuffer.allocate(DEFAULT_CHUNK_SIZE);

, it improves the performance up to 3x time on files of size 1Gb. What if instead, we add the next parameter to the method 'downloadTo' to give the user a chance to set the buffer size on his own?

mcantrell · 2019-01-28T17:52:46Z

The increased byte buffer size is kind of a band-aid. To achieve reasonable download speeds, I'll have to limit the number of concurrent downloads that can be achieved. We've moved from a speed problem to a memory problem.

frankyn · 2019-02-05T17:23:38Z

This fell through my email. Apologies for the delay. @mcantrell this is a trade-off between the two.

IIUC what you were asking for was removing the need to make multiple GET requests to a single GET request to mitigate the overhead. Is this correct?

frankyn · 2019-02-05T20:23:03Z

Thanks @andrey-qlogic, given the new helper method downloadTo(OutputStream) WDYT about removing the code around useDirectDownload? The issue I see is that there are two overloads with different download expectations.

...d-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

mcantrell · 2019-02-06T13:42:44Z

This fell through my email. Apologies for the delay. @mcantrell this is a trade-off between the two.

IIUC what you were asking for was removing the need to make multiple GET requests to a single GET request to mitigate the overhead. Is this correct?

Not exactly. I wanted reasonable performance compared to the deprecated client. I think it's a mistake to trade download performance for memory performance. This is a huge downgrade for us.

The multiple requests appears to be the cause. I would suggest again that resumable downloads is a better pattern for retries than what is currently implemented.

mcantrell · 2019-02-06T13:46:29Z

Let's step back and talk about the practical implication of the proposed fix. To achieve reasonable download performance, I need 10x the memory. That means that for every 10 concurrent downloads I could handle before, I can now only handle 1. I'll need 10x the compute engines to handle out peak traffic.

mcantrell · 2019-02-06T15:48:18Z

Sorry, I missed the comment regarding commit. Just to be clear (there are a lot of threads going on here), we're talking about using a resume instead of increased buffer size to fix the issue?

…unitTest.

…nException.

codecov · 2019-02-11T20:01:10Z

Codecov Report

Merging #4337 into master will decrease coverage by <.01%.
The diff coverage is 38.09%.

@@             Coverage Diff              @@
##             master    #4337      +/-   ##
============================================
- Coverage     49.15%   49.15%   -0.01%     
- Complexity    21934    21936       +2     
============================================
  Files          2077     2077              
  Lines        207174   207216      +42     
  Branches      24099    24100       +1     
============================================
+ Hits         101841   101857      +16     
- Misses        97160    97186      +26     
  Partials       8173     8173

Impacted Files	Coverage Δ	Complexity Δ
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`62.85% <ø> (ø)`	`0 <0> (ø)`	⬇️
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.83% <0%> (-0.06%)`	`1 <0> (ø)`
...ud/storage/contrib/nio/testing/FakeStorageRpc.java	`63.36% <0%> (-0.32%)`	`40 <0> (ø)`
...e/src/main/java/com/google/cloud/storage/Blob.java	`82.32% <84.21%> (+0.22%)`	`32 <2> (+2)`	⬆️

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 07ab941...c7a8938. Read the comment docs.

sduskis · 2019-02-26T14:14:41Z

@andrey-qlogic can you please answer this comment: #4337 (comment)

JustinBeckwith · 2019-02-28T16:59:56Z

gentle ping

andrey-qlogic · 2019-02-28T17:10:17Z

@JustinBeckwith, @sduskis, @mcantrell . That is correct, the PR is about using a resumable download with Http MediaDownloader instead of increase buffer size to fix the issue.

andrey-qlogic · 2019-03-07T10:49:40Z

@sduskis , PTAL

sduskis · 2019-03-08T19:46:50Z

@mcantrell, can you PTAL? Your question was answered. @JesseLovelace or @frankyn: would you be able to review this?

sduskis · 2019-03-19T18:44:12Z

@JesseLovelace or @frankyn: would you be able to review this?

sduskis · 2019-04-30T13:55:29Z

@frankyn, this PR is stale. I'm closing it. Please reach out offline if you would like to restore this PR.

franDiazBitmover · 2019-06-06T13:38:37Z

Hey there! Any news? I see this was closed like one month ago and I'm wondering whether this is addressed on another PR or there are plans to fix this at all...

ajaaym · 2019-06-06T13:49:39Z

@franDiazBitmover we closed this pr without merging it. We will have a new pr early next week addressing this issue.

andrey-qlogic requested a review from a team as a code owner January 14, 2019 18:48

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 14, 2019

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 15, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 15, 2019

andrey-qlogic changed the title ~~3929~~ 3929 Added new downloadToPathWithMediaHttpDownloader method with better performance. Jan 15, 2019

andrey-qlogic changed the title ~~3929 Added new downloadToPathWithMediaHttpDownloader method with better performance.~~ 3929 Slow download performance for Storage API. Added new downloadToPathWithMediaHttpDownloader method with better performance. Jan 15, 2019

ajaaym added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 15, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 15, 2019

frankyn requested changes Jan 18, 2019

View reviewed changes

andrey-qlogic added 2 commits January 21, 2019 14:21

3929: Added downloadToPathWithMediaHttpDownloader method to Blob class

bf11479

3929: Added period at the end of the sentence

dc468d4

andrey-qlogic force-pushed the 3929 branch from e17f03b to dc468d4 Compare January 21, 2019 15:48

andrey-qlogic added 2 commits January 24, 2019 19:15

3929: Added option USE_DIRECT_DOWNLOAD. Prepared tests.

aba57f9

Merge branch 'master' into 3929

9ecd65b

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 24, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 24, 2019

3929: Fixed typo.

110be9a

frankyn requested changes Jan 24, 2019

View reviewed changes

3929: Changes after review.

6d09fae

andrey-qlogic mentioned this pull request Feb 5, 2019

Slow download performance for Storage API #3929

Closed

romlogic reviewed Feb 5, 2019

View reviewed changes

...d-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java Outdated Show resolved Hide resolved

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java Outdated Show resolved Hide resolved

andrey-qlogic self-assigned this Feb 6, 2019

JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019

yoshi-automation removed the 🚨 This issue needs some love. label Feb 7, 2019

andrey-qlogic added 3 commits February 7, 2019 17:22

3929: WIP after reviwew. Removed useDirectDownload lines and added a …

c057fa9

…unitTest.

3929: Reverted changes in local pom files used for testing.

ac984c2

3929: Added implementation to FakeStorageRpc with UnsupportedOperatio…

a8c5c13

…nException.

yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 11, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 11, 2019

andrey-qlogic added 3 commits February 21, 2019 09:59

Merge remote-tracking branch 'upstream/master' into 3929

f1266d8

Added an unit test to StorageImplTest class

3873ea5

WIP: Work on unit tests and codecoverage

c7a8938

sduskis added the api: storage Issues related to the Cloud Storage API. label Mar 19, 2019

sduskis closed this Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3929 Slow download performance for Storage API. Added new downloadToPathWithMediaHttpDownloader method with better performance. #4337

3929 Slow download performance for Storage API. Added new downloadToPathWithMediaHttpDownloader method with better performance. #4337

andrey-qlogic commented Jan 14, 2019 •

edited

Loading

andrey-qlogic commented Jan 14, 2019 •

edited

Loading

sduskis commented Jan 15, 2019

mcantrell commented Jan 15, 2019

andrey-qlogic commented Jan 16, 2019

mcantrell commented Jan 17, 2019

mcantrell commented Jan 17, 2019 •

edited

Loading

andrey-qlogic commented Jan 18, 2019

frankyn Jan 18, 2019 •

edited

Loading

andrey-qlogic Jan 24, 2019

frankyn left a comment •

edited

Loading

andrey-qlogic commented Jan 24, 2019

andrey-qlogic commented Jan 25, 2019 •

edited

Loading

mcantrell commented Jan 28, 2019

frankyn commented Feb 5, 2019

frankyn commented Feb 5, 2019

mcantrell commented Feb 6, 2019

mcantrell commented Feb 6, 2019

mcantrell commented Feb 6, 2019

codecov bot commented Feb 11, 2019 •

edited

Loading

sduskis commented Feb 26, 2019

JustinBeckwith commented Feb 28, 2019

andrey-qlogic commented Feb 28, 2019

andrey-qlogic commented Mar 7, 2019

sduskis commented Mar 8, 2019

sduskis commented Mar 19, 2019

sduskis commented Apr 30, 2019

franDiazBitmover commented Jun 6, 2019 •

edited

Loading

ajaaym commented Jun 6, 2019

3929 Slow download performance for Storage API. Added new downloadToPathWithMediaHttpDownloader method with better performance. #4337

3929 Slow download performance for Storage API. Added new downloadToPathWithMediaHttpDownloader method with better performance. #4337

Conversation

andrey-qlogic commented Jan 14, 2019 • edited Loading

andrey-qlogic commented Jan 14, 2019 • edited Loading

sduskis commented Jan 15, 2019

mcantrell commented Jan 15, 2019

andrey-qlogic commented Jan 16, 2019

mcantrell commented Jan 17, 2019

mcantrell commented Jan 17, 2019 • edited Loading

andrey-qlogic commented Jan 18, 2019

frankyn Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

andrey-qlogic Jan 24, 2019

Choose a reason for hiding this comment

frankyn left a comment • edited Loading

Choose a reason for hiding this comment

andrey-qlogic commented Jan 24, 2019

andrey-qlogic commented Jan 25, 2019 • edited Loading

mcantrell commented Jan 28, 2019

frankyn commented Feb 5, 2019

frankyn commented Feb 5, 2019

mcantrell commented Feb 6, 2019

mcantrell commented Feb 6, 2019

mcantrell commented Feb 6, 2019

codecov bot commented Feb 11, 2019 • edited Loading

Codecov Report

sduskis commented Feb 26, 2019

JustinBeckwith commented Feb 28, 2019

andrey-qlogic commented Feb 28, 2019

andrey-qlogic commented Mar 7, 2019

sduskis commented Mar 8, 2019

sduskis commented Mar 19, 2019

sduskis commented Apr 30, 2019

franDiazBitmover commented Jun 6, 2019 • edited Loading

ajaaym commented Jun 6, 2019

andrey-qlogic commented Jan 14, 2019 •

edited

Loading

andrey-qlogic commented Jan 14, 2019 •

edited

Loading

mcantrell commented Jan 17, 2019 •

edited

Loading

frankyn Jan 18, 2019 •

edited

Loading

frankyn left a comment •

edited

Loading

andrey-qlogic commented Jan 25, 2019 •

edited

Loading

codecov bot commented Feb 11, 2019 •

edited

Loading

franDiazBitmover commented Jun 6, 2019 •

edited

Loading