feat: update cloudstorageconfiguration to improve copy accross cross-bucket performance #168

suraj-qlogic · 2020-07-09T06:16:10Z

Fixes #60

codecov · 2020-07-09T06:21:30Z

Codecov Report

Merging #168 into master will increase coverage by 0.66%.
The diff coverage is 88.23%.

@@             Coverage Diff              @@
##             master     #168      +/-   ##
============================================
+ Coverage     71.13%   71.79%   +0.66%     
- Complexity      481      495      +14     
============================================
  Files            29       29              
  Lines          1642     1656      +14     
  Branches        265      268       +3     
============================================
+ Hits           1168     1189      +21     
+ Misses          348      342       -6     
+ Partials        126      125       -1

Impacted Files	Coverage Δ	Complexity Δ
...ud/storage/contrib/nio/CloudStorageFileSystem.java	`72.60% <88.23%> (+6.50%)`	`36.00 <9.00> (+9.00)`
...ge/contrib/nio/CloudStorageFileSystemProvider.java	`62.91% <0.00%> (+0.46%)`	`76.00% <0.00%> (+3.00%)`
...age/contrib/nio/SeekableByteChannelPrefetcher.java	`66.25% <0.00%> (+3.12%)`	`23.00% <0.00%> (+2.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65d448e...8559a74. Read the comment docs.

elharo · 2020-07-09T12:52:35Z

...cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java

+      assertNotEquals(sourceFileSystem.config(), destFileSystem.config());
+      assertEquals("bucket", sourceFileSystem.bucket());
+      assertEquals("new-bucket", destFileSystem.bucket());
+    } catch (IOException e) {


IOException should fail test; i.e. don't catch it

dmitry-fa

Your change does not fix the original issue which requests not creating a new CloudStorageFileSystem for a bucket and a provider, if such file system has been already created.

To achieve that you need to remember all created file systems and before creating a new one in the forBucket() method do search among previously created.

The code could look like:

private static final Map<String, CloudStorageFileSystem> fileSystemMap = new HashMap<>();
...

public static CloudStorageFileSystem forBucket(
      String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) {
   ...
   CloudStorageFileSystem  existing = getExisitngCloudStorageFileSystem(bucket, config, storageOptions);
   if (existing == null) {
      exiting = new CloudStorageFileSystem (bucket, config, storageOptions);
      fileSystemMap.put(bucket, existing);
   }
   return existing;
}

dmitry-fa · 2020-07-12T20:19:37Z

google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java

+    return new CloudStorageFileSystem(
+        cloudStorageFileSystem.provider(), bucketName, cloudStorageFileSystem.config());
+  }
+


Method name getExistingCloudStorageConfiguration implies returning existing configuration, you return FileSystem and create it each time.

dmitry-fa · 2020-07-12T20:22:02Z

google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java

@@ -195,6 +211,7 @@ public static CloudStorageFileSystem forBucket(
    }
    this.provider = provider;
    this.config = config;
+    this.cloudStorageFileSystem = this;
  }



this.cloudStorageFileSystem = this does not work as you probably expect. Each time a new CloudStorageFileSystem object is created the static field cloudStorageFileSystem will be overridden.

…mance

suraj-qlogic · 2020-07-21T10:52:05Z

@dmitry-fa @frankyn ,PTAL

...cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java

dmitry-fa

Looks good to me, thanks for addressing all my comments. Please wait for a review from a Googler

suraj-qlogic · 2020-07-27T16:31:34Z

@frankyn ,gentle ping

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 9, 2020

suraj-qlogic changed the title ~~Storage nio 60~~ feat: update cloudstorageconfiguration to improve copy accross cross-bucket performance Jul 9, 2020

suraj-qlogic added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2020

athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2020

suraj-qlogic requested review from frankyn and elharo July 9, 2020 07:12

elharo reviewed Jul 9, 2020

View reviewed changes

suraj-qlogic requested a review from elharo July 9, 2020 15:06

elharo approved these changes Jul 9, 2020

View reviewed changes

dmitry-fa suggested changes Jul 12, 2020

View reviewed changes

suraj-qlogic added 3 commits July 21, 2020 10:50

feat: update cloudstorageconfiguration to improve cross-bucket perfor…

c2f5ac2

…mance

feat: update javadoc

997b2b6

feat: fix review changes

8811b6c

suraj-qlogic force-pushed the storage-nio-60 branch from f463c31 to 8811b6c Compare July 21, 2020 05:21

suraj-qlogic self-assigned this Jul 21, 2020

feat: fix review changes

d3c8b8b

suraj-qlogic force-pushed the storage-nio-60 branch from 1f3643b to d3c8b8b Compare July 21, 2020 05:34

suraj-qlogic requested a review from dmitry-fa July 21, 2020 05:36

dmitry-fa reviewed Jul 21, 2020

View reviewed changes

...cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java Outdated Show resolved Hide resolved

...cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java Outdated Show resolved Hide resolved

dmitry-fa self-requested a review July 21, 2020 12:08

feat: fix review changes

8559a74

dmitry-fa approved these changes Jul 21, 2020

View reviewed changes

elharo approved these changes Jul 22, 2020

View reviewed changes

frankyn approved these changes Jul 27, 2020

View reviewed changes

frankyn merged commit db74524 into googleapis:master Jul 27, 2020

suraj-qlogic deleted the storage-nio-60 branch July 30, 2020 13:10

cjllanwarne mentioned this pull request Sep 13, 2021

Storage API: Inappropriate caching and re-use of differently-authenticated filesystem providers #698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update cloudstorageconfiguration to improve copy accross cross-bucket performance #168

feat: update cloudstorageconfiguration to improve copy accross cross-bucket performance #168

suraj-qlogic commented Jul 9, 2020

codecov bot commented Jul 9, 2020 •

edited

Loading

elharo Jul 9, 2020

suraj-qlogic Jul 9, 2020

dmitry-fa left a comment

dmitry-fa Jul 12, 2020

suraj-qlogic Jul 21, 2020

dmitry-fa Jul 12, 2020

suraj-qlogic Jul 21, 2020

suraj-qlogic commented Jul 21, 2020

dmitry-fa left a comment

suraj-qlogic commented Jul 27, 2020

feat: update cloudstorageconfiguration to improve copy accross cross-bucket performance #168

feat: update cloudstorageconfiguration to improve copy accross cross-bucket performance #168

Conversation

suraj-qlogic commented Jul 9, 2020

codecov bot commented Jul 9, 2020 • edited Loading

Codecov Report

elharo Jul 9, 2020

Choose a reason for hiding this comment

suraj-qlogic Jul 9, 2020

Choose a reason for hiding this comment

dmitry-fa left a comment

Choose a reason for hiding this comment

dmitry-fa Jul 12, 2020

Choose a reason for hiding this comment

suraj-qlogic Jul 21, 2020

Choose a reason for hiding this comment

dmitry-fa Jul 12, 2020

Choose a reason for hiding this comment

suraj-qlogic Jul 21, 2020

Choose a reason for hiding this comment

suraj-qlogic commented Jul 21, 2020

dmitry-fa left a comment

Choose a reason for hiding this comment

suraj-qlogic commented Jul 27, 2020

codecov bot commented Jul 9, 2020 •

edited

Loading