From c2f5ac25fc741c9de740a278c878f7019ad73d16 Mon Sep 17 00:00:00 2001 From: suraj dhamecha Date: Tue, 7 Jul 2020 21:09:24 +0530 Subject: [PATCH 1/5] feat: update cloudstorageconfiguration to improve cross-bucket performance --- .../contrib/nio/CloudStorageFileSystem.java | 31 ++++++++++---- .../nio/CloudStorageFileSystemTest.java | 40 +++++++++++++++++++ .../storage/contrib/nio/it/ITGcsNio.java | 32 +++++++++++++++ 3 files changed, 96 insertions(+), 7 deletions(-) diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index b7860b3e..444d44fd 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -61,10 +61,10 @@ public final class CloudStorageFileSystem extends FileSystem { public static final int BLOCK_SIZE_DEFAULT = 2 * 1024 * 1024; public static final FileTime FILE_TIME_UNKNOWN = FileTime.fromMillis(0); public static final Set SUPPORTED_VIEWS = ImmutableSet.of(BASIC_VIEW, GCS_VIEW); - private final CloudStorageFileSystemProvider provider; private final String bucket; private final CloudStorageConfiguration config; + private static CloudStorageFileSystem cloudStorageFileSystem; // Users can change this: then this affects every filesystem object created // later, including via SPI. This is meant to be done only once, at the beginning @@ -134,6 +134,18 @@ public static CloudStorageFileSystem forBucket(String bucket) { return forBucket(bucket, userSpecifiedDefault); } + /** + * Creates new file system instance for {@code bucket}, with existing provider and configuration. + * + * @param bucketName the name of the bucket to initialize CloudStorageFileSystem Object. + * @return CloudStorageFileSystem Object with existing provider and config. + * @see #forBucket(String) + */ + private static CloudStorageFileSystem getExistingCloudStorageConfiguration(String bucketName) { + return new CloudStorageFileSystem( + cloudStorageFileSystem.provider(), bucketName, cloudStorageFileSystem.config()); + } + /** * Creates new file system instance for {@code bucket}, with customizable settings. * @@ -144,8 +156,10 @@ public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfig checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); checkNotNull(config); - return new CloudStorageFileSystem( - new CloudStorageFileSystemProvider(config.userProject()), bucket, config); + return (cloudStorageFileSystem != null && cloudStorageFileSystem.config().equals(config)) + ? getExistingCloudStorageConfiguration(bucket) + : new CloudStorageFileSystem( + new CloudStorageFileSystemProvider(config.userProject()), bucket, config); } /** @@ -168,10 +182,12 @@ public static CloudStorageFileSystem forBucket( String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) { checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - return new CloudStorageFileSystem( - new CloudStorageFileSystemProvider(config.userProject(), storageOptions), - bucket, - checkNotNull(config)); + return (cloudStorageFileSystem != null && cloudStorageFileSystem.config().equals(config)) + ? getExistingCloudStorageConfiguration(bucket) + : new CloudStorageFileSystem( + new CloudStorageFileSystemProvider(config.userProject(), storageOptions), + bucket, + checkNotNull(config)); } CloudStorageFileSystem( @@ -195,6 +211,7 @@ public static CloudStorageFileSystem forBucket( } this.provider = provider; this.config = config; + this.cloudStorageFileSystem = this; } @Override diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java index e46ab752..301a8807 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java @@ -18,6 +18,9 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import com.google.cloud.storage.StorageOptions; import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper; @@ -334,6 +337,43 @@ public void testDeleteRecursive() throws IOException { } } + @Test + public void testSameProvider() { + try (CloudStorageFileSystem sourceFileSystem = + CloudStorageFileSystem.forBucket( + "bucket", + CloudStorageConfiguration.builder().permitEmptyPathComponents(true).build())) { + CloudStorageFileSystem destFileSystem = + CloudStorageFileSystem.forBucket( + "new-bucket", + CloudStorageConfiguration.builder().permitEmptyPathComponents(true).build()); + assertEquals(sourceFileSystem.provider(), destFileSystem.provider()); + assertEquals(sourceFileSystem.config(), destFileSystem.config()); + assertEquals("bucket", sourceFileSystem.bucket()); + assertEquals("new-bucket", destFileSystem.bucket()); + } catch (IOException e) { + + } + } + + @Test + public void testDifferentProvider() { + try (CloudStorageFileSystem sourceFileSystem = + CloudStorageFileSystem.forBucket( + "bucket", + CloudStorageConfiguration.builder().permitEmptyPathComponents(true).build())) { + CloudStorageFileSystem destFileSystem = + CloudStorageFileSystem.forBucket( + "new-bucket", + CloudStorageConfiguration.builder().permitEmptyPathComponents(false).build()); + assertFalse(sourceFileSystem.provider() == destFileSystem.provider()); + assertNotEquals(sourceFileSystem.config(), destFileSystem.config()); + assertEquals("bucket", sourceFileSystem.bucket()); + assertEquals("new-bucket", destFileSystem.bucket()); + } catch (IOException e) { + // fail + } + } /** * Delete the given directory and all of its contents if non-empty. * diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 6f3293eb..8ed96eb9 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -20,6 +20,8 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import com.google.api.client.http.HttpResponseException; import com.google.cloud.storage.BlobInfo; @@ -96,6 +98,7 @@ public class ITGcsNio { private static final Logger log = Logger.getLogger(ITGcsNio.class.getName()); private static final String BUCKET = RemoteStorageHelper.generateBucketName(); + private static final String TARGET_BUCKET = RemoteStorageHelper.generateBucketName(); private static final String REQUESTER_PAYS_BUCKET = RemoteStorageHelper.generateBucketName() + "_rp"; private static final String SML_FILE = "tmp-test-small-file.txt"; @@ -119,6 +122,7 @@ public static void beforeClass() throws IOException { storage = storageOptions.getService(); // create and populate test bucket storage.create(BucketInfo.of(BUCKET)); + storage.create(BucketInfo.of(TARGET_BUCKET)); fillFile(storage, BUCKET, SML_FILE, SML_SIZE); fillFile(storage, BUCKET, BIG_FILE, BIG_SIZE); BucketInfo requesterPaysBucket = @@ -1040,6 +1044,34 @@ public ImmutableList getPaths() { } } + @Test + public void testCopyWithSameProvider() throws IOException { + CloudStorageFileSystem sourceFileSystem = getTestBucket(); + CloudStorageFileSystem targetFileSystem = + CloudStorageFileSystem.forBucket( + TARGET_BUCKET, CloudStorageConfiguration.DEFAULT, storageOptions); + Path sourceFileSystemPath = sourceFileSystem.getPath(SML_FILE); + Path targetFileSystemPath = targetFileSystem.getPath(PREFIX + randomSuffix()); + Files.copy(sourceFileSystemPath, targetFileSystemPath); + assertEquals(sourceFileSystem.provider(), targetFileSystem.provider()); + assertEquals(sourceFileSystem.config(), targetFileSystem.config()); + } + + @Test + public void testCopyWithDifferentProvider() throws IOException { + CloudStorageFileSystem sourceFileSystem = getTestBucket(); + CloudStorageFileSystem targetFileSystem = + CloudStorageFileSystem.forBucket( + TARGET_BUCKET, + CloudStorageConfiguration.builder().permitEmptyPathComponents(true).build(), + storageOptions); + Path sourceFileSystemPath = sourceFileSystem.getPath(SML_FILE); + Path targetFileSystemPath = targetFileSystem.getPath(PREFIX + randomSuffix()); + Files.copy(sourceFileSystemPath, targetFileSystemPath); + assertThat(sourceFileSystem.provider() == targetFileSystem.provider()).isFalse(); + assertNotEquals(sourceFileSystem.config(), targetFileSystem.config()); + } + private CloudStorageFileSystem getTestBucket() throws IOException { // in typical usage we use the single-argument version of forBucket // and rely on the user being logged into their project with the From 997b2b639accafb885c63668e7097d325dfde8e2 Mon Sep 17 00:00:00 2001 From: suraj dhamecha Date: Thu, 9 Jul 2020 11:32:00 +0530 Subject: [PATCH 2/5] feat: update javadoc --- .../cloud/storage/contrib/nio/CloudStorageFileSystem.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index 444d44fd..496a5181 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -137,8 +137,8 @@ public static CloudStorageFileSystem forBucket(String bucket) { /** * Creates new file system instance for {@code bucket}, with existing provider and configuration. * - * @param bucketName the name of the bucket to initialize CloudStorageFileSystem Object. - * @return CloudStorageFileSystem Object with existing provider and config. + * @param bucketName name of the bucket to initialize {@code CloudStorageFileSystem} object + * @return {@code CloudStorageFileSystem} object with existing provider and config * @see #forBucket(String) */ private static CloudStorageFileSystem getExistingCloudStorageConfiguration(String bucketName) { From 8811b6c832d976e76282b6145954f3abb7c1c43d Mon Sep 17 00:00:00 2001 From: suraj dhamecha Date: Thu, 9 Jul 2020 20:28:20 +0530 Subject: [PATCH 3/5] feat: fix review changes --- .../storage/contrib/nio/CloudStorageFileSystemTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java index 301a8807..f378f7aa 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java @@ -338,7 +338,7 @@ public void testDeleteRecursive() throws IOException { } @Test - public void testSameProvider() { + public void testSameProvider() throws IOException { try (CloudStorageFileSystem sourceFileSystem = CloudStorageFileSystem.forBucket( "bucket", @@ -351,13 +351,11 @@ public void testSameProvider() { assertEquals(sourceFileSystem.config(), destFileSystem.config()); assertEquals("bucket", sourceFileSystem.bucket()); assertEquals("new-bucket", destFileSystem.bucket()); - } catch (IOException e) { - } } @Test - public void testDifferentProvider() { + public void testDifferentProvider() throws IOException { try (CloudStorageFileSystem sourceFileSystem = CloudStorageFileSystem.forBucket( "bucket", @@ -370,8 +368,6 @@ public void testDifferentProvider() { assertNotEquals(sourceFileSystem.config(), destFileSystem.config()); assertEquals("bucket", sourceFileSystem.bucket()); assertEquals("new-bucket", destFileSystem.bucket()); - } catch (IOException e) { - // fail } } /** From d3c8b8bfa7b473d322f725b4340b159477c2d3fd Mon Sep 17 00:00:00 2001 From: suraj dhamecha Date: Tue, 21 Jul 2020 10:56:41 +0530 Subject: [PATCH 4/5] feat: fix review changes --- .../contrib/nio/CloudStorageFileSystem.java | 53 ++++++++++--------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index 496a5181..8c1a13d7 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -38,6 +38,8 @@ import java.nio.file.attribute.FileTime; import java.nio.file.attribute.UserPrincipalLookupService; import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; import java.util.Objects; import java.util.Set; import javax.annotation.CheckReturnValue; @@ -64,7 +66,8 @@ public final class CloudStorageFileSystem extends FileSystem { private final CloudStorageFileSystemProvider provider; private final String bucket; private final CloudStorageConfiguration config; - private static CloudStorageFileSystem cloudStorageFileSystem; + private static final Map> + CONFIG_TO_PROVIDERS_MAP = new HashMap<>(); // Users can change this: then this affects every filesystem object created // later, including via SPI. This is meant to be done only once, at the beginning @@ -134,18 +137,6 @@ public static CloudStorageFileSystem forBucket(String bucket) { return forBucket(bucket, userSpecifiedDefault); } - /** - * Creates new file system instance for {@code bucket}, with existing provider and configuration. - * - * @param bucketName name of the bucket to initialize {@code CloudStorageFileSystem} object - * @return {@code CloudStorageFileSystem} object with existing provider and config - * @see #forBucket(String) - */ - private static CloudStorageFileSystem getExistingCloudStorageConfiguration(String bucketName) { - return new CloudStorageFileSystem( - cloudStorageFileSystem.provider(), bucketName, cloudStorageFileSystem.config()); - } - /** * Creates new file system instance for {@code bucket}, with customizable settings. * @@ -156,10 +147,29 @@ public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfig checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); checkNotNull(config); - return (cloudStorageFileSystem != null && cloudStorageFileSystem.config().equals(config)) - ? getExistingCloudStorageConfiguration(bucket) - : new CloudStorageFileSystem( - new CloudStorageFileSystemProvider(config.userProject()), bucket, config); + return new CloudStorageFileSystem( + getCloudStorageFileSystemProvider(config, null), bucket, config); + } + + private static CloudStorageFileSystemProvider getCloudStorageFileSystemProvider( + CloudStorageConfiguration config, StorageOptions storageOptions) { + CloudStorageFileSystemProvider newProvider = + (storageOptions == null) + ? new CloudStorageFileSystemProvider(config.userProject()) + : new CloudStorageFileSystemProvider(config.userProject(), storageOptions); + Set existingProviders = CONFIG_TO_PROVIDERS_MAP.get(config); + if (existingProviders == null) { + existingProviders = new HashSet<>(); + } else { + for (CloudStorageFileSystemProvider existiningProvider : existingProviders) { + if (existiningProvider.equals(newProvider)) { + return existiningProvider; + } + } + } + existingProviders.add(newProvider); + CONFIG_TO_PROVIDERS_MAP.put(config, existingProviders); + return newProvider; } /** @@ -182,12 +192,8 @@ public static CloudStorageFileSystem forBucket( String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) { checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - return (cloudStorageFileSystem != null && cloudStorageFileSystem.config().equals(config)) - ? getExistingCloudStorageConfiguration(bucket) - : new CloudStorageFileSystem( - new CloudStorageFileSystemProvider(config.userProject(), storageOptions), - bucket, - checkNotNull(config)); + return new CloudStorageFileSystem( + getCloudStorageFileSystemProvider(config, storageOptions), bucket, checkNotNull(config)); } CloudStorageFileSystem( @@ -211,7 +217,6 @@ public static CloudStorageFileSystem forBucket( } this.provider = provider; this.config = config; - this.cloudStorageFileSystem = this; } @Override From 8559a748bb467e929e5dad7f7ad4c998d08a0cdc Mon Sep 17 00:00:00 2001 From: suraj dhamecha Date: Tue, 21 Jul 2020 18:22:40 +0530 Subject: [PATCH 5/5] feat: fix review changes --- .../storage/contrib/nio/CloudStorageFileSystemTest.java | 7 ++++--- .../com/google/cloud/storage/contrib/nio/it/ITGcsNio.java | 6 ++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java index f378f7aa..966bf994 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java @@ -19,8 +19,9 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; import com.google.cloud.storage.StorageOptions; import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper; @@ -347,7 +348,7 @@ public void testSameProvider() throws IOException { CloudStorageFileSystem.forBucket( "new-bucket", CloudStorageConfiguration.builder().permitEmptyPathComponents(true).build()); - assertEquals(sourceFileSystem.provider(), destFileSystem.provider()); + assertSame(sourceFileSystem.provider(), destFileSystem.provider()); assertEquals(sourceFileSystem.config(), destFileSystem.config()); assertEquals("bucket", sourceFileSystem.bucket()); assertEquals("new-bucket", destFileSystem.bucket()); @@ -364,7 +365,7 @@ public void testDifferentProvider() throws IOException { CloudStorageFileSystem.forBucket( "new-bucket", CloudStorageConfiguration.builder().permitEmptyPathComponents(false).build()); - assertFalse(sourceFileSystem.provider() == destFileSystem.provider()); + assertNotSame(sourceFileSystem.provider(), destFileSystem.provider()); assertNotEquals(sourceFileSystem.config(), destFileSystem.config()); assertEquals("bucket", sourceFileSystem.bucket()); assertEquals("new-bucket", destFileSystem.bucket()); diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 8ed96eb9..d44d2705 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -22,6 +22,8 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; import com.google.api.client.http.HttpResponseException; import com.google.cloud.storage.BlobInfo; @@ -1053,7 +1055,7 @@ public void testCopyWithSameProvider() throws IOException { Path sourceFileSystemPath = sourceFileSystem.getPath(SML_FILE); Path targetFileSystemPath = targetFileSystem.getPath(PREFIX + randomSuffix()); Files.copy(sourceFileSystemPath, targetFileSystemPath); - assertEquals(sourceFileSystem.provider(), targetFileSystem.provider()); + assertSame(sourceFileSystem.provider(), targetFileSystem.provider()); assertEquals(sourceFileSystem.config(), targetFileSystem.config()); } @@ -1068,7 +1070,7 @@ public void testCopyWithDifferentProvider() throws IOException { Path sourceFileSystemPath = sourceFileSystem.getPath(SML_FILE); Path targetFileSystemPath = targetFileSystem.getPath(PREFIX + randomSuffix()); Files.copy(sourceFileSystemPath, targetFileSystemPath); - assertThat(sourceFileSystem.provider() == targetFileSystem.provider()).isFalse(); + assertNotSame(sourceFileSystem.provider(), targetFileSystem.provider()); assertNotEquals(sourceFileSystem.config(), targetFileSystem.config()); }