From 0f2b337900e8d47eee8823f11f689728dc6ae97d Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 26 Oct 2018 11:31:02 -0700 Subject: [PATCH 1/9] Put new retry options in CloudStorageConfiguration Add retriableHttpCodes and reopenableExceptions so the user can, if they choose, customize the conditions under which we retry an access or reopen a channel after an exception closed it. This can be handy if a cloud provider starts transiently throwing exceptions that would normally indicate a fatal condition, but which in practice will pass if retried. The backoff logic in retries / reopens in unmodified. --- .../nio/CloudStorageConfiguration.java | 42 +++++++++++++++- .../nio/CloudStorageFileSystemProvider.java | 21 +++++--- .../contrib/nio/CloudStorageReadChannel.java | 13 +++-- .../contrib/nio/CloudStorageRetryHandler.java | 48 +++++++++++-------- .../nio/CloudStorageConfigurationTest.java | 11 +++++ .../nio/CloudStorageReadChannelTest.java | 4 +- 6 files changed, 104 insertions(+), 35 deletions(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java index a0e699b07005..4a03fd9a4071 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java @@ -20,8 +20,13 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import javax.annotation.Nullable; +import javax.net.ssl.SSLException; +import java.io.EOFException; +import java.net.SocketException; +import java.net.SocketTimeoutException; import java.util.Map; /** @@ -89,6 +94,17 @@ public abstract class CloudStorageConfiguration { */ public abstract boolean useUserProjectOnlyForRequesterPaysBuckets(); + /** + * Returns the set of HTTP error codes that will be retried, in addition to the normally + * retriable ones. + */ + public abstract ImmutableList retriableHttpCodes(); + + /** + * Returns the set of exceptions for which we'll try a channel reopen if maxChannelReopens + * is positive. + */ + public abstract ImmutableList> reopenableExceptions(); /** * Creates a new builder, initialized with the following settings: @@ -118,6 +134,10 @@ public static final class Builder { private @Nullable String userProject = null; // This of this as "clear userProject if not RequesterPays" private boolean useUserProjectOnlyForRequesterPaysBuckets = false; + private ImmutableList retriableHttpCodes=ImmutableList.of(500, 502, 503); + private ImmutableList> reopenableExceptions = + ImmutableList.>of( + SSLException.class, EOFException.class, SocketException.class, SocketTimeoutException.class); /** * Changes current working directory for new filesystem. This defaults to the root directory. @@ -186,6 +206,16 @@ public Builder autoDetectRequesterPays(boolean value) { return this; } + public Builder retriableHttpCodes(ImmutableList value) { + retriableHttpCodes = value; + return this; + } + + public Builder reopenableExceptions(ImmutableList> values) { + reopenableExceptions = values; + return this; + } + /** * Creates new instance without destroying builder. */ @@ -198,7 +228,9 @@ public CloudStorageConfiguration build() { blockSize, maxChannelReopens, userProject, - useUserProjectOnlyForRequesterPaysBuckets); + useUserProjectOnlyForRequesterPaysBuckets, + retriableHttpCodes, + reopenableExceptions); } Builder(CloudStorageConfiguration toModify) { @@ -210,6 +242,8 @@ public CloudStorageConfiguration build() { maxChannelReopens = toModify.maxChannelReopens(); userProject = toModify.userProject(); useUserProjectOnlyForRequesterPaysBuckets = toModify.useUserProjectOnlyForRequesterPaysBuckets(); + retriableHttpCodes = toModify.retriableHttpCodes(); + reopenableExceptions = toModify.reopenableExceptions(); } Builder() {} @@ -250,6 +284,12 @@ static private CloudStorageConfiguration fromMap(Builder builder, Map case "useUserProjectOnlyForRequesterPaysBuckets": builder.autoDetectRequesterPays((Boolean) entry.getValue()); break; + case "retriableHttpCodes": + builder.retriableHttpCodes((ImmutableList) entry.getValue()); + break; + case "reopenableExceptions": + builder.reopenableExceptions((ImmutableList>) entry.getValue()); + break; default: throw new IllegalArgumentException(entry.getKey()); } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java index a4e56c0c875d..fa549fd835bd 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java @@ -309,6 +309,7 @@ private SeekableByteChannel newReadChannel(Path path, Set throws IOException { initStorage(); int maxChannelReopens = CloudStorageUtil.getMaxChannelReopensFromPath(path); + int[] retriableHttpCodes = new int[]{}; List blobSourceOptions = new ArrayList<>(); for (OpenOption option : options) { if (option instanceof StandardOpenOption) { @@ -349,6 +350,7 @@ private SeekableByteChannel newReadChannel(Path path, Set cloudPath.getBlobId(), 0, maxChannelReopens, + cloudPath.getFileSystem().config(), userProject, blobSourceOptions.toArray(new BlobSourceOption[blobSourceOptions.size()])); } @@ -461,7 +463,8 @@ public boolean deleteIfExists(Path path) throws IOException { throw new CloudStoragePseudoDirectoryException(cloudPath); } - final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(CloudStorageUtil.getMaxChannelReopensFromPath(path)); + final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler( + cloudPath.getFileSystem().config()); // Loop will terminate via an exception if all retries are exhausted while (true) { try { @@ -586,7 +589,8 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep throw new CloudStoragePseudoDirectoryException(toPath); } - final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(CloudStorageUtil.getMaxChannelReopensFromPath(source)); + final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler( + fromPath.getFileSystem().config()); // Loop will terminate via an exception if all retries are exhausted while (true) { try { @@ -672,11 +676,12 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { } } - final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(CloudStorageUtil.getMaxChannelReopensFromPath(path)); + final CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); + final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler( + cloudPath.getFileSystem().config()); // Loop will terminate via an exception if all retries are exhausted while (true) { try { - CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); boolean nullId; if (isNullOrEmpty(userProject)) { nullId = storage.get( @@ -725,11 +730,12 @@ public A readAttributes( } initStorage(); - final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(CloudStorageUtil.getMaxChannelReopensFromPath(path)); + final CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); + final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler( + cloudPath.getFileSystem().config()); // Loop will terminate via an exception if all retries are exhausted while (true) { try { - CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); BlobInfo blobInfo = null; try { BlobId blobId = cloudPath.getBlobId(); @@ -811,7 +817,8 @@ public DirectoryStream newDirectoryStream(final Path dir, final Filter 0) { - if ((throwable.getMessage() != null - && throwable.getMessage().contains("Connection closed prematurely")) - || throwable instanceof SSLException - || throwable instanceof EOFException - || throwable instanceof SocketException - || throwable instanceof SocketTimeoutException) { + for (Class retriableException : config.reopenableExceptions()) { + if (retriableException.isInstance(throwable)) { + return true; + } + } + if (throwable.getMessage() != null + && throwable.getMessage().contains("Connection closed prematurely")) { return true; } throwable = throwable.getCause(); diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java index b4fc68d036a1..cb13e7b34ac9 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.junit.Rule; @@ -26,6 +27,8 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import java.net.SocketTimeoutException; + /** * Unit tests for {@link CloudStorageConfiguration}. */ @@ -43,12 +46,16 @@ public void testBuilder() { .stripPrefixSlash(false) .usePseudoDirectories(false) .blockSize(666) + .retriableHttpCodes(ImmutableList.of(1,2,3)) + .reopenableExceptions(ImmutableList.>of(SocketTimeoutException.class)) .build(); assertThat(config.workingDirectory()).isEqualTo("/omg"); assertThat(config.permitEmptyPathComponents()).isTrue(); assertThat(config.stripPrefixSlash()).isFalse(); assertThat(config.usePseudoDirectories()).isFalse(); assertThat(config.blockSize()).isEqualTo(666); + assertThat(config.retriableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); + assertThat(config.reopenableExceptions()).isEqualTo(ImmutableList.>of(SocketTimeoutException.class)); } @Test @@ -61,12 +68,16 @@ public void testFromMap() { .put("stripPrefixSlash", false) .put("usePseudoDirectories", false) .put("blockSize", 666) + .put("retriableHttpCodes", ImmutableList.of(1,2,3)) + .put("reopenableExceptions", ImmutableList.>of(SocketTimeoutException.class)) .build()); assertThat(config.workingDirectory()).isEqualTo("/omg"); assertThat(config.permitEmptyPathComponents()).isTrue(); assertThat(config.stripPrefixSlash()).isFalse(); assertThat(config.usePseudoDirectories()).isFalse(); assertThat(config.blockSize()).isEqualTo(666); + assertThat(config.retriableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); + assertThat(config.reopenableExceptions()).isEqualTo(ImmutableList.>of(SocketTimeoutException.class)); } @Test diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java index 292ddff7e727..b8416e21572a 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java @@ -67,7 +67,7 @@ public void before() throws IOException { when(gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE))).thenReturn(metadata); when(gcsStorage.reader(file, Storage.BlobSourceOption.generationMatch(2L))).thenReturn(gcsChannel); when(gcsChannel.isOpen()).thenReturn(true); - chan = CloudStorageReadChannel.create(gcsStorage, file, 0, 1, ""); + chan = CloudStorageReadChannel.create(gcsStorage, file, 0, 1, CloudStorageConfiguration.DEFAULT, ""); verify(gcsStorage).get(eq(file), eq(Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE))); verify(gcsStorage).reader(eq(file), eq(Storage.BlobSourceOption.generationMatch(2L))); } @@ -208,7 +208,7 @@ public void testChannelPositionDoesNotGetTruncatedToInt() throws IOException { // Invoke CloudStorageReadChannel.create() to trigger a call to the private // CloudStorageReadChannel.innerOpen() method, which does a seek on our gcsChannel. - CloudStorageReadChannel.create(gcsStorage, file, startPosition, 1, ""); + CloudStorageReadChannel.create(gcsStorage, file, startPosition, 1, CloudStorageConfiguration.DEFAULT, ""); // Confirm that our position did not overflow during the seek in CloudStorageReadChannel.innerOpen() verify(gcsChannel).seek(captor.capture()); From 4736a3c6ebea108d2722c1f693144997a99dd946 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 26 Oct 2018 13:15:05 -0700 Subject: [PATCH 2/9] Clean up dead code line --- .../storage/contrib/nio/CloudStorageFileSystemProvider.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java index fa549fd835bd..a4ae5d9686fc 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java @@ -309,7 +309,6 @@ private SeekableByteChannel newReadChannel(Path path, Set throws IOException { initStorage(); int maxChannelReopens = CloudStorageUtil.getMaxChannelReopensFromPath(path); - int[] retriableHttpCodes = new int[]{}; List blobSourceOptions = new ArrayList<>(); for (OpenOption option : options) { if (option instanceof StandardOpenOption) { From ef37bb3784b054c773e499ec7da63f9d8634f937 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 29 Oct 2018 10:17:21 -0700 Subject: [PATCH 3/9] retriable -> retryable (in NIO Cloud Storage code) Also add a few basic tests to make sure the retryable options do what we expect. --- .../nio/CloudStorageConfiguration.java | 18 ++++++++--------- .../contrib/nio/CloudStorageRetryHandler.java | 17 +++++++++------- .../nio/CloudStorageConfigurationTest.java | 19 ++++++++++++++---- .../nio/CloudStorageRetryHandlerTest.java | 20 +++++++++++++++++++ 4 files changed, 54 insertions(+), 20 deletions(-) create mode 100644 google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java index 4a03fd9a4071..f4fb24f29d8b 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java @@ -96,9 +96,9 @@ public abstract class CloudStorageConfiguration { /** * Returns the set of HTTP error codes that will be retried, in addition to the normally - * retriable ones. + * retryable ones. */ - public abstract ImmutableList retriableHttpCodes(); + public abstract ImmutableList retryableHttpCodes(); /** * Returns the set of exceptions for which we'll try a channel reopen if maxChannelReopens @@ -134,7 +134,7 @@ public static final class Builder { private @Nullable String userProject = null; // This of this as "clear userProject if not RequesterPays" private boolean useUserProjectOnlyForRequesterPaysBuckets = false; - private ImmutableList retriableHttpCodes=ImmutableList.of(500, 502, 503); + private ImmutableList retryableHttpCodes = ImmutableList.of(500, 502, 503); private ImmutableList> reopenableExceptions = ImmutableList.>of( SSLException.class, EOFException.class, SocketException.class, SocketTimeoutException.class); @@ -206,8 +206,8 @@ public Builder autoDetectRequesterPays(boolean value) { return this; } - public Builder retriableHttpCodes(ImmutableList value) { - retriableHttpCodes = value; + public Builder retryableHttpCodes(ImmutableList value) { + retryableHttpCodes = value; return this; } @@ -229,7 +229,7 @@ public CloudStorageConfiguration build() { maxChannelReopens, userProject, useUserProjectOnlyForRequesterPaysBuckets, - retriableHttpCodes, + retryableHttpCodes, reopenableExceptions); } @@ -242,7 +242,7 @@ public CloudStorageConfiguration build() { maxChannelReopens = toModify.maxChannelReopens(); userProject = toModify.userProject(); useUserProjectOnlyForRequesterPaysBuckets = toModify.useUserProjectOnlyForRequesterPaysBuckets(); - retriableHttpCodes = toModify.retriableHttpCodes(); + retryableHttpCodes = toModify.retryableHttpCodes(); reopenableExceptions = toModify.reopenableExceptions(); } @@ -284,8 +284,8 @@ static private CloudStorageConfiguration fromMap(Builder builder, Map case "useUserProjectOnlyForRequesterPaysBuckets": builder.autoDetectRequesterPays((Boolean) entry.getValue()); break; - case "retriableHttpCodes": - builder.retriableHttpCodes((ImmutableList) entry.getValue()); + case "retryableHttpCodes": + builder.retryableHttpCodes((ImmutableList) entry.getValue()); break; case "reopenableExceptions": builder.reopenableExceptions((ImmutableList>) entry.getValue()); diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java index 0f1e4be9dda4..a7c91ae3de13 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java @@ -17,6 +17,7 @@ package com.google.cloud.storage.contrib.nio; import com.google.cloud.storage.StorageException; +import com.google.common.annotations.VisibleForTesting; /** * Simple counter class to keep track of retry and reopen attempts when StorageExceptions are @@ -34,7 +35,7 @@ public class CloudStorageRetryHandler { /** * Create a CloudStorageRetryHandler with the maximum retries and reopens set to the same value. * - * @param config - configuration for reopens and retriable codes. + * @param config - configuration for reopens and retryable codes. */ public CloudStorageRetryHandler(final CloudStorageConfiguration config) { this.maxRetries = config.maxChannelReopens(); @@ -140,13 +141,14 @@ void sleepForAttempt(int attempt) { /** * @param exs StorageException to test - * @return true if exs is a retriable error, otherwise false + * @return true if exs is a retryable error, otherwise false */ - private boolean isRetryable(final StorageException exs) { + @VisibleForTesting + boolean isRetryable(final StorageException exs) { if (exs.isRetryable()) { return true; } - for (int code : config.retriableHttpCodes()) { + for (int code : config.retryableHttpCodes()) { if (exs.getCode() == code) { return true; } @@ -158,13 +160,14 @@ private boolean isRetryable(final StorageException exs) { * @param exs StorageException to test * @return true if exs is an error that can be resolved via a channel reopen, otherwise false */ - private boolean isReopenable(final StorageException exs) { + @VisibleForTesting + boolean isReopenable(final StorageException exs) { Throwable throwable = exs; // ensures finite iteration int maxDepth = 20; while (throwable != null && maxDepth-- > 0) { - for (Class retriableException : config.reopenableExceptions()) { - if (retriableException.isInstance(throwable)) { + for (Class reopenableException : config.reopenableExceptions()) { + if (reopenableException.isInstance(throwable)) { return true; } } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java index cb13e7b34ac9..6548b446c52f 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageConfigurationTest.java @@ -46,7 +46,7 @@ public void testBuilder() { .stripPrefixSlash(false) .usePseudoDirectories(false) .blockSize(666) - .retriableHttpCodes(ImmutableList.of(1,2,3)) + .retryableHttpCodes(ImmutableList.of(1,2,3)) .reopenableExceptions(ImmutableList.>of(SocketTimeoutException.class)) .build(); assertThat(config.workingDirectory()).isEqualTo("/omg"); @@ -54,7 +54,7 @@ public void testBuilder() { assertThat(config.stripPrefixSlash()).isFalse(); assertThat(config.usePseudoDirectories()).isFalse(); assertThat(config.blockSize()).isEqualTo(666); - assertThat(config.retriableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); + assertThat(config.retryableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); assertThat(config.reopenableExceptions()).isEqualTo(ImmutableList.>of(SocketTimeoutException.class)); } @@ -68,7 +68,7 @@ public void testFromMap() { .put("stripPrefixSlash", false) .put("usePseudoDirectories", false) .put("blockSize", 666) - .put("retriableHttpCodes", ImmutableList.of(1,2,3)) + .put("retryableHttpCodes", ImmutableList.of(1,2,3)) .put("reopenableExceptions", ImmutableList.>of(SocketTimeoutException.class)) .build()); assertThat(config.workingDirectory()).isEqualTo("/omg"); @@ -76,7 +76,7 @@ public void testFromMap() { assertThat(config.stripPrefixSlash()).isFalse(); assertThat(config.usePseudoDirectories()).isFalse(); assertThat(config.blockSize()).isEqualTo(666); - assertThat(config.retriableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); + assertThat(config.retryableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); assertThat(config.reopenableExceptions()).isEqualTo(ImmutableList.>of(SocketTimeoutException.class)); } @@ -85,4 +85,15 @@ public void testFromMap_badKey_throwsIae() { thrown.expect(IllegalArgumentException.class); CloudStorageConfiguration.fromMap(ImmutableMap.of("lol", "/omg")); } + + @Test + /** + * Spot check that our defaults are applied. + */ + public void testSomeDefaults() { + for (CloudStorageConfiguration config : ImmutableList.of(CloudStorageConfiguration.DEFAULT, CloudStorageConfiguration.builder().build())) { + assertThat(config.retryableHttpCodes()).contains(503); + assertThat(config.reopenableExceptions()).contains(SocketTimeoutException.class); + } + } } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java new file mode 100644 index 000000000000..9b102a0cf6fc --- /dev/null +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java @@ -0,0 +1,20 @@ +package com.google.cloud.storage.contrib.nio; + +import org.junit.Test; +import com.google.cloud.storage.StorageException; +import com.google.common.collect.ImmutableList; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +import static com.google.common.truth.Truth.assertThat; + +@RunWith(JUnit4.class) +public class CloudStorageRetryHandlerTest { + + @Test + public void testIsRetryable() throws Exception { + CloudStorageConfiguration config = CloudStorageConfiguration.builder().retryableHttpCodes(ImmutableList.of(1,2,3)).build(); + CloudStorageRetryHandler retrier = new CloudStorageRetryHandler(config); + assertThat(retrier.isRetryable( new StorageException(1, "pretend error code 1"))).isTrue(); + } +} From 1a034327b43732fd1e11454e931bf0ed6d579a26 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 29 Oct 2018 10:38:08 -0700 Subject: [PATCH 4/9] Update testListFilesInRootDirectory to match reality In the built-in UNIX filesystem, the newDirectoryStream method will return absolute paths if given absolute paths as input, and relative paths if given relative paths as input. We're now seeing this too for the NIO Cloud Storage provider (this is a good thing), so I updated this test to reflect this new reality. --- .../cloud/storage/contrib/nio/it/ITGcsNio.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 08e75c1137fe..ad2b06067767 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -54,6 +54,7 @@ import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; @@ -615,11 +616,17 @@ public void testListFilesInRootDirectory() throws IOException { // test absolute path, relative path. for (String check : new String[]{".", "/", ""}) { Path rootPath = fs.getPath(check); - List objectNames = new ArrayList<>(); + List pathsFound = new ArrayList<>(); for (Path path : Files.newDirectoryStream(rootPath)) { - objectNames.add(path.toString()); + // The returned paths will match the absolute-ness of the root path + // (this matches the behavior of the built-in UNIX file system). + assertWithMessage("Absolute/relative for " + check + ": ") + .that(path.isAbsolute()).isEqualTo(rootPath.isAbsolute()); + // To simplify the check that we found our files, we normalize here. + pathsFound.add(path.toAbsolutePath()); } - assertWithMessage("Listing " + check + ": ").that(objectNames).containsExactly(BIG_FILE, SML_FILE); + assertWithMessage("Listing " + check + ": ").that(pathsFound).containsExactly( + fs.getPath(BIG_FILE).toAbsolutePath(), fs.getPath(SML_FILE).toAbsolutePath()); } } From eab4881431fcd9529a32ad97284123dc30a8b6a6 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 30 Oct 2018 14:12:17 -0700 Subject: [PATCH 5/9] remove unused import --- .../java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index ad2b06067767..91e9728ff213 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -54,7 +54,6 @@ import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; From c309bad55646bc104518ded68a29189e9e2c9b66 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 2 Nov 2018 14:23:09 -0700 Subject: [PATCH 6/9] Add boilerplate file header --- .../nio/CloudStorageRetryHandlerTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java index 9b102a0cf6fc..c31648acd836 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandlerTest.java @@ -1,3 +1,19 @@ +/* + * Copyright 2018 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.cloud.storage.contrib.nio; import org.junit.Test; From 8474dcb40008e3c36fe2910c184e882ad66aa728 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 12 Nov 2018 15:07:16 -0800 Subject: [PATCH 7/9] Keep old ctor, just @deprecated --- .../contrib/nio/CloudStorageRetryHandler.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java index a7c91ae3de13..7e4f41239796 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java @@ -32,6 +32,21 @@ public class CloudStorageRetryHandler { private final int maxReopens; private final CloudStorageConfiguration config; + /** + * Create a CloudStorageRetryHandler with the maximum retries and reopens set to different values. + * + * @param maxRetries maximum number of retries + * @param maxReopens maximum number of reopens + * + * @deprecated use CloudStorageRetryHandler(CloudStorageConfiguration) instead. + */ + public CloudStorageRetryHandler(final int maxRetries, final int maxReopens) { + this.maxRetries = maxRetries; + this.maxReopens = maxReopens; + // we're just using the retry parameters from the config, so it's OK to have a default. + this.config = CloudStorageConfiguration.DEFAULT; + } + /** * Create a CloudStorageRetryHandler with the maximum retries and reopens set to the same value. * From 40d473c66c4662024852ffe210b959af188815f5 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 12 Nov 2018 15:17:27 -0800 Subject: [PATCH 8/9] Add back the *other* ctor that needs to be deprecated --- .../contrib/nio/CloudStorageRetryHandler.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java index 7e4f41239796..58c2b6f47181 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java @@ -32,7 +32,22 @@ public class CloudStorageRetryHandler { private final int maxReopens; private final CloudStorageConfiguration config; + /** + * Create a CloudStorageRetryHandler with the maximum retries and reopens set to the same value. + * + * @param maxRetriesAndReopens value for both maxRetries and maxReopens + * + * @deprecated use CloudStorageRetryHandler(CloudStorageConfiguration) instead. + */ + public CloudStorageRetryHandler(final int maxRetriesAndReopens) { + this.maxRetries = maxRetriesAndReopens; + this.maxReopens = maxRetriesAndReopens; + // we're just using the retry parameters from the config, so it's OK to have a default. + this.config = CloudStorageConfiguration.DEFAULT; + } + + /** * Create a CloudStorageRetryHandler with the maximum retries and reopens set to different values. * * @param maxRetries maximum number of retries From 0eb2adb948fbe0e53bd36b521ef646b68773ee54 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 13 Nov 2018 10:07:16 -0800 Subject: [PATCH 9/9] add @deprecated outside of comment --- .../cloud/storage/contrib/nio/CloudStorageRetryHandler.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java index 58c2b6f47181..83b2573471a4 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageRetryHandler.java @@ -40,6 +40,7 @@ public class CloudStorageRetryHandler { * * @deprecated use CloudStorageRetryHandler(CloudStorageConfiguration) instead. */ + @java.lang.Deprecated public CloudStorageRetryHandler(final int maxRetriesAndReopens) { this.maxRetries = maxRetriesAndReopens; this.maxReopens = maxRetriesAndReopens; @@ -55,6 +56,7 @@ public CloudStorageRetryHandler(final int maxRetriesAndReopens) { * * @deprecated use CloudStorageRetryHandler(CloudStorageConfiguration) instead. */ + @java.lang.Deprecated public CloudStorageRetryHandler(final int maxRetries, final int maxReopens) { this.maxRetries = maxRetries; this.maxReopens = maxReopens; @@ -77,7 +79,7 @@ public CloudStorageRetryHandler(final CloudStorageConfiguration config) { * Create a CloudStorageRetryHandler with the maximum retries and reopens set to different values. * * @param maxRetries maximum number of retries - * @param maxReopens maximum number of reopens + * @param maxReopens maximum number of reopens (overrides what's in the config) * @param config http codes we'll retry on, and exceptions we'll reopen on. */ public CloudStorageRetryHandler(final int maxRetries, final int maxReopens, final CloudStorageConfiguration config) {