From c2f5ac25fc741c9de740a278c878f7019ad73d16 Mon Sep 17 00:00:00 2001 From: suraj dhamecha Date: Tue, 7 Jul 2020 21:09:24 +0530 Subject: [PATCH] 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