cloud-storage-contrib-nio: Add support for newFileChannel by olavloite · Pull Request #4718 · googleapis/google-cloud-java

olavloite · 2019-03-22T06:39:29Z

Adds support for the method FileSystemProvider.newFileChannel(Path, Set, FileAttribute...) to Google Cloud Storage nio. This enables, among others, users to use google-cloud-storage-nio with Webflux and MultipartFile upload.

Fixes #4702

sduskis · 2019-03-22T18:38:26Z

Can you please run this: mvn com.coveo:fmt-maven-plugin:format

codecov · 2019-03-22T18:49:51Z

Codecov Report

Merging #4718 into master will increase coverage by 0.69%.
The diff coverage is 63.47%.

@@             Coverage Diff              @@
##             master    #4718      +/-   ##
============================================
+ Coverage     49.66%   50.35%   +0.69%     
- Complexity    22706    23667     +961     
============================================
  Files          2158     2233      +75     
  Lines        213520   225890   +12370     
  Branches      24206    24958     +752     
============================================
+ Hits         106045   113758    +7713     
- Misses        99183   103533    +4350     
- Partials       8292     8599     +307

Impacted Files	Coverage Δ	Complexity Δ
...ge/contrib/nio/CloudStorageFileSystemProvider.java	`61.26% <30.76%> (-1.21%)`	`70 <1> (+1)`
...rage/contrib/nio/CloudStorageWriteFileChannel.java	`64.91% <64.91%> (ø)`	`13 <13> (?)`
...orage/contrib/nio/CloudStorageReadFileChannel.java	`71.11% <71.11%> (ø)`	`10 <10> (?)`
...cloud/iam/credentials/v1/IamCredentialsClient.java	`46.22% <0%> (-32.26%)`	`23% <0%> (+3%)`
...oud/automl/v1beta1/stub/PredictionServiceStub.java	`20% <0%> (-30%)`	`1% <0%> (ø)`
...ava/com/google/cloud/storage/BlobWriteChannel.java	`71.66% <0%> (-19.51%)`	`9% <0%> (+4%)`
...urityscanner/v1alpha/WebSecurityScannerClient.java	`66.5% <0%> (-16.04%)`	`55% <0%> (+7%)`
...d/storage/CanonicalExtensionHeadersSerializer.java	`88.88% <0%> (-11.12%)`	`12% <0%> (+4%)`
...va/com/google/cloud/redis/v1/CloudRedisClient.java	`53.33% <0%> (-7.03%)`	`27% <0%> (-8%)`
.../cloud/monitoring/v3/UptimeCheckServiceClient.java	`54.45% <0%> (-4.44%)`	`23% <0%> (ø)`
... and 234 more

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 db18d86...5c2c732. Read the comment docs.

JesseLovelace · 2019-04-05T21:11:02Z

+   * @param path The path to the file to open or create
+   * @param options The options specifying how the file should be opened, and whether the {@link
+   *     FileChannel} should be read-only or write-only.
+   * @param attrs (not supported, the values will be ignored)


Why the null check for attrs below if this is the case? attrs is optional in File.createFile

I chose to do so to keep it consistent with the other methods in CloudStorageFileSystemProvider that accept an array of FileAtributes as an input parameter. These do the exact same check, even though the input value is ignored.

(See newByteChannel(...)``createDirectory(...))

Furthermore, newFileChannel method can also be used to read an existing file, so it does not always call File.createFile.

sduskis · 2019-03-27T18:19:07Z

+          (CloudStorageWriteChannel) newWriteChannel(path, options);
+      return new CloudStorageWriteFileChannel(writeChannel);
+    } else {
+      CloudStorageReadChannel readChannel = (CloudStorageReadChannel) newReadChannel(path, options);


Do you need to cast to a CloudStorageReadChannel here? if not perhaps just do this: new CloudStorageReadFileChannel(newReadChannel(path, options));

Good point, changed.

sduskis · 2019-04-07T13:32:01Z

+  @Override
+  public long read(ByteBuffer[] dsts, int offset, int length) throws IOException {
+    long res = 0L;
+    synchronized (this) {


You might as well jut put synchronized in the signature.

sduskis · 2019-04-07T13:32:34Z

+  @Override
+  public long transferTo(long position, long count, WritableByteChannel target) throws IOException {
+    long res = 0L;
+    synchronized (this) {


synchronized in the signature might be clearer.

sduskis · 2019-04-07T13:32:56Z

+
+  @Override
+  public int read(ByteBuffer dst, long position) throws IOException {
+    synchronized (this) {


sduskis · 2019-04-10T14:09:07Z

+  public int read(ByteBuffer dst, long position) throws IOException {
+    synchronized (this) {
+      long originalPosition = position();
+      position(position);


Isn't this redundant?

long originalPosition = position(); position(position);

Do we actually need position(position);?

Yes, we need to position the channel where the read should begin. I've changed the name of the parameter to make it clearer.

sduskis · 2019-04-10T14:10:44Z

+  public FileChannel position(long newPosition) throws IOException {
+    synchronized (this) {
+      checkValid();
+      if (newPosition != position()) {


There's a call to checkValid() in position(). We don't need two calls. Perhaps just use writeChannel.position()

There are other places where the double check happens as well, further down.

I removed the redundant validation checks and changed the method signatures in this class as well to synchronized.

sduskis

Looking good. I think that my last couple request ought to be done, but once they are, please merge this.

sduskis · 2019-04-11T13:17:42Z

+        res += bytesRead;
+      }
+    }
+    position(originalPosition);


Please put position(originalPosition) in a finally block.

sduskis · 2019-04-11T13:17:47Z

+    long originalPosition = position();
+    position(readFromPosition);
+    int res = readChannel.read(dst);
+    position(originalPosition);


Please put position(originalPosition) in a finally block.

olavloite added 2 commits March 21, 2019 08:21

add CloudStorageFileChannel

ec543ea

googleapis#4702 add support for newFileChannel(...)

c46e001

olavloite requested a review from a team March 22, 2019 06:39

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 22, 2019

removed erroneously committed file

831dbf7

fixed formatting

7f38d08

added synchronization to file channel classes

e87fbc9

kolea2 reviewed Mar 27, 2019

View reviewed changes

Comment thread ...loud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadFileChannel.java Outdated

changed copyright years and removed unnecessary formatting changes

b08147d

allycatdaboss approved these changes Mar 28, 2019

View reviewed changes

yoshi-automation added the 🚨 critical P0 critical issue. Requires immediate fix label Mar 29, 2019

JesseLovelace reviewed Apr 5, 2019

View reviewed changes

sduskis reviewed Apr 7, 2019

View reviewed changes

removed unnecessary cast

693f852

sduskis suggested changes Apr 10, 2019

View reviewed changes

olavloite added 3 commits April 10, 2019 16:52

make methods synchronized + clearify parameter

0561c82

make methods synchronized + remove redundant validation

cf73070

fixed formatting

8d1a382

sduskis approved these changes Apr 11, 2019

View reviewed changes

sduskis added 🚨 critical P0 critical issue. Requires immediate fix and removed 🚨 critical P0 critical issue. Requires immediate fix labels Apr 11, 2019

put position(originalPosition) in finally-block

5c2c732

olavloite merged commit b697a8b into googleapis:master Apr 12, 2019

olavloite deleted the nio-filechannel branch April 12, 2019 20:46

kolea2 mentioned this pull request Apr 17, 2019

Release google-cloud-java v0.88.0 #4971

Merged

Conversation

olavloite commented Mar 22, 2019

Uh oh!

sduskis commented Mar 22, 2019

Uh oh!

codecov Bot commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

JesseLovelace Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sduskis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Mar 22, 2019 •

edited

Loading

JesseLovelace Apr 5, 2019 •

edited

Loading