From 324085e830c35debde0b5e8d89bc629496481b27 Mon Sep 17 00:00:00 2001 From: JP Martin Date: Tue, 27 Nov 2018 14:02:47 -0800 Subject: [PATCH] Storage NIO: Add new retry options in CloudStorageConfiguration (#3869) * 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. * Clean up dead code line * retriable -> retryable (in NIO Cloud Storage code) Also add a few basic tests to make sure the retryable options do what we expect. * 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. * remove unused import * Add boilerplate file header * Keep old ctor, just @deprecated * Add back the *other* ctor that needs to be deprecated * add @deprecated outside of comment --- .../nio/CloudStorageConfiguration.java | 42 ++++++++++- .../nio/CloudStorageFileSystemProvider.java | 20 +++-- .../contrib/nio/CloudStorageReadChannel.java | 13 ++-- .../contrib/nio/CloudStorageRetryHandler.java | 73 +++++++++++++++---- .../nio/CloudStorageConfigurationTest.java | 22 ++++++ .../nio/CloudStorageReadChannelTest.java | 4 +- .../nio/CloudStorageRetryHandlerTest.java | 36 +++++++++ .../storage/contrib/nio/it/ITGcsNio.java | 12 ++- 8 files changed, 189 insertions(+), 33 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 a0e699b07005..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 @@ -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 + * retryable ones. + */ + public abstract ImmutableList retryableHttpCodes(); + + /** + * 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 retryableHttpCodes = 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 retryableHttpCodes(ImmutableList value) { + retryableHttpCodes = 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, + retryableHttpCodes, + reopenableExceptions); } Builder(CloudStorageConfiguration toModify) { @@ -210,6 +242,8 @@ public CloudStorageConfiguration build() { maxChannelReopens = toModify.maxChannelReopens(); userProject = toModify.userProject(); useUserProjectOnlyForRequesterPaysBuckets = toModify.useUserProjectOnlyForRequesterPaysBuckets(); + retryableHttpCodes = toModify.retryableHttpCodes(); + reopenableExceptions = toModify.reopenableExceptions(); } Builder() {} @@ -250,6 +284,12 @@ static private CloudStorageConfiguration fromMap(Builder builder, Map case "useUserProjectOnlyForRequesterPaysBuckets": builder.autoDetectRequesterPays((Boolean) entry.getValue()); break; + case "retryableHttpCodes": + builder.retryableHttpCodes((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 701a0d854ab2..ef025920818a 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 @@ -349,6 +349,7 @@ private SeekableByteChannel newReadChannel(Path path, Set cloudPath.getBlobId(), 0, maxChannelReopens, + cloudPath.getFileSystem().config(), userProject, blobSourceOptions.toArray(new BlobSourceOption[blobSourceOptions.size()])); } @@ -461,7 +462,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 +588,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 +675,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 +729,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 +816,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 reopenableException : config.reopenableExceptions()) { + if (reopenableException.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..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 @@ -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) + .retryableHttpCodes(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.retryableHttpCodes()).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("retryableHttpCodes", 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.retryableHttpCodes()).isEqualTo(ImmutableList.of(1,2,3)); + assertThat(config.reopenableExceptions()).isEqualTo(ImmutableList.>of(SocketTimeoutException.class)); } @Test @@ -74,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/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()); 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..c31648acd836 --- /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,36 @@ +/* + * 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; +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(); + } +} 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 7b11f24d87c3..ed5b687148f0 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 @@ -636,11 +636,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()); } }