Skip to content

Commit

Permalink
feat: update cloudstorageconfiguration to improve cross-bucket perfor…
Browse files Browse the repository at this point in the history
…mance
  • Loading branch information
suraj-qlogic committed Jul 21, 2020
1 parent 65d448e commit c2f5ac2
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 7 deletions.
Expand Up @@ -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<String> 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
Expand Down Expand Up @@ -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.
*
Expand All @@ -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);
}

/**
Expand All @@ -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(
Expand All @@ -195,6 +211,7 @@ public static CloudStorageFileSystem forBucket(
}
this.provider = provider;
this.config = config;
this.cloudStorageFileSystem = this;
}

@Override
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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";
Expand All @@ -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 =
Expand Down Expand Up @@ -1040,6 +1044,34 @@ public ImmutableList<Path> 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
Expand Down

0 comments on commit c2f5ac2

Please sign in to comment.