From c00cb4ccbd3f10a3765144e1ca99924e5530d92e Mon Sep 17 00:00:00 2001 From: Ajay Date: Fri, 19 Jul 2019 10:43:35 -0400 Subject: [PATCH 1/8] download blob using direct download method --- .../java/com/google/cloud/storage/Blob.java | 53 ++++++++++++++----- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java b/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java index 722df42e9477..7179de681068 100644 --- a/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java +++ b/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java @@ -16,9 +16,11 @@ package com.google.cloud.storage; +import static com.google.cloud.RetryHelper.runWithRetries; import static com.google.cloud.storage.Blob.BlobSourceOption.toGetOptions; import static com.google.cloud.storage.Blob.BlobSourceOption.toSourceOptions; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.concurrent.Executors.callable; import com.google.api.services.storage.model.StorageObject; import com.google.auth.ServiceAccountSigner; @@ -34,13 +36,11 @@ import com.google.cloud.storage.spi.v1.StorageRpc; import com.google.common.base.Function; import com.google.common.io.BaseEncoding; +import com.google.common.io.CountingOutputStream; import java.io.IOException; import java.io.ObjectInputStream; import java.io.OutputStream; import java.net.URL; -import java.nio.ByteBuffer; -import java.nio.channels.Channels; -import java.nio.channels.WritableByteChannel; import java.nio.file.Files; import java.nio.file.Path; import java.security.Key; @@ -211,20 +211,49 @@ static Storage.BlobGetOption[] toGetOptions(BlobInfo blobInfo, BlobSourceOption. * @throws StorageException upon failure */ public void downloadTo(Path path, BlobSourceOption... options) { - try (OutputStream outputStream = Files.newOutputStream(path); - ReadChannel reader = reader(options)) { - WritableByteChannel channel = Channels.newChannel(outputStream); - ByteBuffer bytes = ByteBuffer.allocate(DEFAULT_CHUNK_SIZE); - while (reader.read(bytes) > 0) { - bytes.flip(); - channel.write(bytes); - bytes.clear(); - } + try (OutputStream outputStream = Files.newOutputStream(path)) { + downloadTo(outputStream, options); } catch (IOException e) { throw new StorageException(e); } } + /** + * Downloads this blob to the given output stream using specified blob read options. + * + * @param outputStream + * @param options + */ + public void downloadTo(OutputStream outputStream, BlobSourceOption... options) { + final CountingOutputStream countingOutputStream = new CountingOutputStream(outputStream); + final StorageRpc storageRpc = this.options.getStorageRpcV1(); + final Map requestOptions = StorageImpl.optionMap(getBlobId(), options); + runWithRetries( + callable( + new Runnable() { + @Override + public void run() { + storageRpc.read( + getBlobId().toPb(), + requestOptions, + countingOutputStream.getCount(), + countingOutputStream); + } + }), + this.options.getRetrySettings(), + StorageImpl.EXCEPTION_HANDLER, + this.options.getClock()); + } + + /** + * Downloads this blob to the given output stream. + * + * @param outputStream + */ + public void downloadTo(OutputStream outputStream) { + downloadTo(outputStream, new BlobSourceOption[0]); + } + /** * Downloads this blob to the given file path. * From 615dab77f53c7cb995e4d6aacb06caf042e6a630 Mon Sep 17 00:00:00 2001 From: Ajay Date: Fri, 19 Jul 2019 12:15:09 -0400 Subject: [PATCH 2/8] update test case --- .../com/google/cloud/storage/BlobTest.java | 64 ++++++++++++++----- 1 file changed, 47 insertions(+), 17 deletions(-) diff --git a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java index d68bf965f1f4..74f6e3d2012e 100644 --- a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java +++ b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java @@ -33,6 +33,9 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import com.google.api.core.ApiClock; +import com.google.api.gax.retrying.RetrySettings; +import com.google.api.services.storage.model.StorageObject; import com.google.cloud.ReadChannel; import com.google.cloud.storage.Acl.Project; import com.google.cloud.storage.Acl.Project.ProjectRole; @@ -41,10 +44,12 @@ import com.google.cloud.storage.Blob.BlobSourceOption; import com.google.cloud.storage.Storage.BlobWriteOption; import com.google.cloud.storage.Storage.CopyRequest; +import com.google.cloud.storage.spi.v1.StorageRpc; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.BaseEncoding; import java.io.File; +import java.io.OutputStream; import java.net.URL; import java.nio.ByteBuffer; import java.nio.file.Files; @@ -58,6 +63,7 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.threeten.bp.Duration; public class BlobTest { @@ -130,6 +136,28 @@ public class BlobTest { 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 = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(100L)) + .setRetryDelayMultiplier(1.3) + .setMaxRetryDelay(Duration.ofMillis(60000L)) + .setInitialRpcTimeout(Duration.ofMillis(20000L)) + .setRpcTimeoutMultiplier(1.0) + .setMaxRpcTimeout(Duration.ofMillis(20000L)) + .setTotalTimeout(Duration.ofMillis(600000L)) + .build(); + private static final ApiClock API_CLOCK = + new ApiClock() { + @Override + public long nanoTime() { + return 42_000_000_000L; + } + + @Override + public long millisTime() { + return 42_000L; + } + }; private Storage storage; private Blob blob; @@ -566,28 +594,30 @@ public void testBuilder() { @Test public void testDownload() throws Exception { final byte[] expected = {1, 2}; - - initializeExpectedBlob(2); - ReadChannel channel = createNiceMock(ReadChannel.class); - expect(storage.getOptions()).andReturn(mockOptions); - expect(storage.reader(BLOB_INFO.getBlobId())).andReturn(channel); + StorageRpc mockStorageRpc = createNiceMock(StorageRpc.class); + expect(storage.getOptions()).andReturn(mockOptions).times(1); replay(storage); - // First read should return 2 bytes. - expect(channel.read(anyObject(ByteBuffer.class))) + expect(mockOptions.getStorageRpcV1()).andReturn(mockStorageRpc); + expect(mockOptions.getRetrySettings()).andReturn(RETRY_SETTINGS); + expect(mockOptions.getClock()).andReturn(API_CLOCK); + replay(mockOptions); + blob = new Blob(storage, new BlobInfo.BuilderImpl(BLOB_INFO)); + expect( + mockStorageRpc.read( + anyObject(StorageObject.class), + anyObject(Map.class), + eq(0l), + anyObject(OutputStream.class) + )) .andAnswer( - new IAnswer() { + new IAnswer() { @Override - public Integer answer() throws Throwable { - // Modify the argument to match the expected behavior of `read`. - ((ByteBuffer) getCurrentArguments()[0]).put(expected); - return 2; + public Long answer() throws Throwable { + ((OutputStream) getCurrentArguments()[3]).write(expected); + return 2l; } }); - // Second read should return 0 bytes. - expect(channel.read(anyObject(ByteBuffer.class))).andReturn(0); - replay(channel); - initializeBlob(); - + replay(mockStorageRpc); File file = File.createTempFile("blob", ".tmp"); blob.downloadTo(file.toPath()); byte actual[] = Files.readAllBytes(file.toPath()); From ae1ffab8a218846984ad9f4f9d61ca19ce96795d Mon Sep 17 00:00:00 2001 From: Ajay Date: Fri, 19 Jul 2019 12:43:26 -0400 Subject: [PATCH 3/8] fix format --- .../test/java/com/google/cloud/storage/BlobTest.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java index 74f6e3d2012e..a5b590a6465e 100644 --- a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java +++ b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java @@ -51,7 +51,6 @@ import java.io.File; import java.io.OutputStream; import java.net.URL; -import java.nio.ByteBuffer; import java.nio.file.Files; import java.security.Key; import java.util.List; @@ -603,12 +602,11 @@ public void testDownload() throws Exception { replay(mockOptions); blob = new Blob(storage, new BlobInfo.BuilderImpl(BLOB_INFO)); expect( - mockStorageRpc.read( - anyObject(StorageObject.class), - anyObject(Map.class), - eq(0l), - anyObject(OutputStream.class) - )) + mockStorageRpc.read( + anyObject(StorageObject.class), + anyObject(Map.class), + eq(0l), + anyObject(OutputStream.class))) .andAnswer( new IAnswer() { @Override From 1bdc86425159e402a17c7d2a88496e3938e984a2 Mon Sep 17 00:00:00 2001 From: Ajay Date: Fri, 19 Jul 2019 13:12:58 -0400 Subject: [PATCH 4/8] add test case for retries --- .../com/google/cloud/storage/BlobTest.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java index a5b590a6465e..efa616054318 100644 --- a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java +++ b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java @@ -621,4 +621,50 @@ public Long answer() throws Throwable { byte actual[] = Files.readAllBytes(file.toPath()); assertArrayEquals(expected, actual); } + + @Test + public void testDownloadWithRetries() throws Exception { + final byte[] expected = {1, 2}; + StorageRpc mockStorageRpc = createNiceMock(StorageRpc.class); + expect(storage.getOptions()).andReturn(mockOptions); + replay(storage); + expect(mockOptions.getStorageRpcV1()).andReturn(mockStorageRpc); + expect(mockOptions.getRetrySettings()).andReturn(RETRY_SETTINGS); + expect(mockOptions.getClock()).andReturn(API_CLOCK); + replay(mockOptions); + blob = new Blob(storage, new BlobInfo.BuilderImpl(BLOB_INFO)); + expect( + mockStorageRpc.read( + anyObject(StorageObject.class), + anyObject(Map.class), + eq(0l), + anyObject(OutputStream.class))) + .andAnswer( + new IAnswer() { + @Override + public Long answer() throws Throwable { + ((OutputStream) getCurrentArguments()[3]).write(expected[0]); + throw new StorageException(504, "error"); + } + }); + expect( + mockStorageRpc.read( + anyObject(StorageObject.class), + anyObject(Map.class), + eq(1l), + anyObject(OutputStream.class))) + .andAnswer( + new IAnswer() { + @Override + public Long answer() throws Throwable { + ((OutputStream) getCurrentArguments()[3]).write(expected[1]); + return 1l; + } + }); + replay(mockStorageRpc); + File file = File.createTempFile("blob", ".tmp"); + blob.downloadTo(file.toPath()); + byte actual[] = Files.readAllBytes(file.toPath()); + assertArrayEquals(expected, actual); + } } From 48777c2e25cc6f3a6fc7fc0ad6913fcc56bccfca Mon Sep 17 00:00:00 2001 From: Ajay Date: Tue, 30 Jul 2019 08:14:44 -0400 Subject: [PATCH 5/8] remove method --- .../src/main/java/com/google/cloud/storage/Blob.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java b/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java index 7179de681068..7cb7e1a91b95 100644 --- a/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java +++ b/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java @@ -245,15 +245,6 @@ public void run() { this.options.getClock()); } - /** - * Downloads this blob to the given output stream. - * - * @param outputStream - */ - public void downloadTo(OutputStream outputStream) { - downloadTo(outputStream, new BlobSourceOption[0]); - } - /** * Downloads this blob to the given file path. * From 0aebf4ca55b897f04ba213e2f7c5b2c89ef92921 Mon Sep 17 00:00:00 2001 From: Ajay Date: Tue, 30 Jul 2019 08:21:04 -0400 Subject: [PATCH 6/8] update test retry setting --- .../src/test/java/com/google/cloud/storage/BlobTest.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java index efa616054318..ea88f958815e 100644 --- a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java +++ b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java @@ -137,13 +137,7 @@ public class BlobTest { new SecretKeySpec(BaseEncoding.base64().decode(BASE64_KEY), "AES256"); private static final RetrySettings RETRY_SETTINGS = RetrySettings.newBuilder() - .setInitialRetryDelay(Duration.ofMillis(100L)) - .setRetryDelayMultiplier(1.3) - .setMaxRetryDelay(Duration.ofMillis(60000L)) - .setInitialRpcTimeout(Duration.ofMillis(20000L)) - .setRpcTimeoutMultiplier(1.0) - .setMaxRpcTimeout(Duration.ofMillis(20000L)) - .setTotalTimeout(Duration.ofMillis(600000L)) + .setMaxAttempts(2) .build(); private static final ApiClock API_CLOCK = new ApiClock() { From 58469f522fd8623b4cdd2fe5000e5bf190a3b935 Mon Sep 17 00:00:00 2001 From: Ajay Date: Tue, 30 Jul 2019 21:50:16 -0400 Subject: [PATCH 7/8] fix lint and add comment --- .../src/test/java/com/google/cloud/storage/BlobTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java index ea88f958815e..983daefaf8aa 100644 --- a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java +++ b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java @@ -62,7 +62,6 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.threeten.bp.Duration; public class BlobTest { @@ -135,6 +134,10 @@ public class BlobTest { private static final String BASE64_KEY = "JVzfVl8NLD9FjedFuStegjRfES5ll5zc59CIXw572OA="; private static final Key KEY = new SecretKeySpec(BaseEncoding.base64().decode(BASE64_KEY), "AES256"); + + // This retrying setting is used by test testDownloadWithRetries. This unit test is setup + // to write one byte and then throw retryable exception, it then writes another bytes on + // second call succeeds. private static final RetrySettings RETRY_SETTINGS = RetrySettings.newBuilder() .setMaxAttempts(2) From cb655191f870de4984700ed6f6b1e78d7533db57 Mon Sep 17 00:00:00 2001 From: Ajay Date: Tue, 30 Jul 2019 22:08:13 -0400 Subject: [PATCH 8/8] fix lint --- .../src/test/java/com/google/cloud/storage/BlobTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java index 983daefaf8aa..6f91f968a03e 100644 --- a/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java +++ b/google-cloud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java @@ -139,9 +139,7 @@ public class BlobTest { // to write one byte and then throw retryable exception, it then writes another bytes on // second call succeeds. private static final RetrySettings RETRY_SETTINGS = - RetrySettings.newBuilder() - .setMaxAttempts(2) - .build(); + RetrySettings.newBuilder().setMaxAttempts(2).build(); private static final ApiClock API_CLOCK = new ApiClock() { @Override