feat: add storage.upload(path) #269

dmitry-fa · 2020-04-17T14:07:52Z

Fixes #179

codecov · 2020-04-17T14:14:52Z

Codecov Report

Merging #269 into master will increase coverage by 0.06%.
The diff coverage is 68.42%.

@@             Coverage Diff              @@
##             master     #269      +/-   ##
============================================
+ Coverage     63.13%   63.19%   +0.06%     
- Complexity      601      609       +8     
============================================
  Files            32       32              
  Lines          5078     5111      +33     
  Branches        481      485       +4     
============================================
+ Hits           3206     3230      +24     
- Misses         1708     1717       +9     
  Partials        164      164

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/com/google/cloud/storage/Storage.java	`80.90% <ø> (ø)`	`0.00 <0.00> (ø)`
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.50% <0.00%> (-0.02%)`	`1.00 <0.00> (ø)`
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`64.86% <ø> (ø)`	`0.00 <0.00> (ø)`
...ogle/cloud/storage/testing/StorageRpcTestBase.java	`96.00% <0.00%> (-1.96%)`	`48.00 <0.00> (ø)`
...ava/com/google/cloud/storage/BlobWriteChannel.java	`74.60% <100.00%> (+2.93%)`	`10.00 <1.00> (+1.00)`
...ain/java/com/google/cloud/storage/StorageImpl.java	`81.00% <100.00%> (+0.47%)`	`138.00 <7.00> (+7.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 37f00eb...d7c7f1b. Read the comment docs.

elharo · 2020-04-17T15:23:25Z

google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java

+
+  @Test
+  public void testUploadDirectory() throws IOException {
+    EasyMock.replay(storageRpcMock);


Please use Mockito for all new tests. It really is better.

@elharo I was going to use Mockito for these new tests, but found this idea not very productive.
I submitted #270 and assigned to myself to fix this.

@elharo, new tests for upload() converted to mockito.
It's not a final version yet. upload() method will be updated to return Blob (now they return void), that will require test update as well.

frankyn

1 nit so far, added @JesseLovelace for surface review as well.

frankyn · 2020-04-17T20:11:17Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * @throws StorageException on failure
+   * @see #upload(BlobInfo, Path, int, BlobWriteOption...)
+   */
+  void upload(BlobInfo blobInfo, Path path, BlobWriteOption... options) throws IOException;


Blob should be returned from upload() methods similar to create() methods.

returning Blob requires an extra RPC request to be issued, that is not always needed.
I can add uploadFrom() method to the Blob class (symmetric to downloadTo) to return an updated Blob.

A month ago I compared upload/download implemented in various ways for several types of file. This is the summary

Thanks @dmitry-fa,

I was thinking about the issue you filed: #149

When the upload is complete GCS responds with object metadata so we don't need to perform an additional get request. flushBuffer doesn't return a value so we'd diverge from the spec a bit to get the metadata.

Do you mean we can update the StorageRpc interface to make void write(uploadId, ...) to return a StorageObject?

1 nit so far, added @JesseLovelace for surface review as well.
Blob should be returned from upload() methods similar to create() methods.

It's not a nit at all :) It's a significant change. I agree with you, upload() should return Blob.
I see the following ways how to implement:

Update the StorageRpc interface and make write() to return a StorageObject instance or null in write is not final request for the given upload. Now StorageRpc .write() returns null.

Update HttpStorageRpc.write() to remember StorageObject and return this object in a separate method.
In this case BlobWriterChannel.fluchBuffer() will look like:

public void run() { StorageRpc rpc = getOptions().getStorageRpcV1(); rpc.write(getUploadId(), getBuffer(), 0, getPosition(), length, last); if (last && rpc instnceof HttpStorageRpc) { this.uploadedBlob = ((HttpStorageRpc)rpc.getUplodedBlob()) } else { this.uploadedBlob = null; // let storage.upload() to resolve it } }

Issue an extra rpc per upload to return a Blob

I believe 2. is not a Google way, so we should go 1. As a temporary solution 3. is also considered, because it could be improved to 1.

WDYT?

Hi @frankyn,

When the upload is complete GCS responds with object metadata so we don't need to perform an additional get request.

I managed to implement upload based on your hint. I modified StorageRpc.write() to return StorageObject instead of void.
Two issues were found:

GCS does not respond with object metadata if writer was created by storage.writer(signedUrl)
Not sure if it's a bug or a feature, looks like a bug: spec says nothing about it. I worked it around.

Change StrorageRpc affects storage-nio, Kokoro - Test: Linkage Monitor fails:
(com.google.cloud:google-cloud-nio:0.120.0-alpha) com.google.cloud.storage.contrib.nio.testing.FakeStorageRpc's method write(String arg1, byte[] arg2, int arg3, long arg4, int arg6, boolean arg7) is not implemented in the class referenced from com.google.cloud.storage.spi.v1.StorageRpc (com.google.cloud:google-cloud-storage:1.107.1-SNAPSHOT)

cc: @JesseLovelace

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

frankyn · 2020-04-17T20:58:27Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * @throws StorageException on failure
+   * @see #upload(BlobInfo, Path, int, BlobWriteOption...)
+   */
+  void upload(BlobInfo blobInfo, Path path, BlobWriteOption... options) throws IOException;


Thanks @dmitry-fa,

I was thinking about the issue you filed: #149

When the upload is complete GCS responds with object metadata so we don't need to perform an additional get request. flushBuffer doesn't return a value so we'd diverge from the spec a bit to get the metadata.

googlebot · 2020-04-21T13:56:23Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-04-21T14:00:59Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

# Conflicts: # pom.xml

frankyn

Thanks @dmitry-fa for your patience.

The more I think about this you may want to use StorageRpc.open() and StorageRpc.write() directly instead of through the writer.

That is an alternative solution there, but it does introduce duplication of code. I'm not convinced that there a clean solution other than what you've done by introducing a new value of objectProto.

As for the linkagefailure, it's known and I'm working on addressing it in #242. Need to address the split in resources here and I'm still working on it.

frankyn · 2020-04-28T05:23:21Z

google-cloud-storage/pom.xml

+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-core</artifactId>
+      <scope>test</scope>
+    </dependency>


Can you split out mockito tests into a separate PR because it got a bit confusing in review? Apologies for churn on this.

Agree. Done #284

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

dmitry-fa · 2020-04-28T10:45:02Z

Hi @frankyn,

Thanks for you comments.

The more I think about this you may want to use StorageRpc.open() and StorageRpc.write() directly instead of through the writer.

That is an alternative solution there, but it does introduce duplication of code. I'm not convinced that there a clean solution other than what you've done by introducing a new value of objectProto.

I agree, objectProto is not elegant solution, but it's better than duplication of BaseWriteChannel code. BaseWriteChannel provides nice and not trivial functionality on buffering (setChunkSize()) which is already tested. Repeating that may lead to new bugs.

frankyn · 2020-04-28T20:23:38Z

Thanks @dmitry-fa, let's move forward for now. It's not introducing new surface methods in the BlobWriteChannel class. Please resolve merge conflicts. This will be blocked until Linkage Monitor is resolved because the StorageRpc update.

dmitry-fa · 2020-04-29T11:23:55Z

Hi @frankyn,

thanks for the prompt response. I have one more question regarding this feature.
Once we add upload() methods to the Storage there will be two groups of methods with similar signatures which do the same job: create() and upload():

  @Deprecated
  Blob create(BlobInfo blobInfo, InputStream content, BlobWriteOption... options);

  Blob upload(BlobInfo blobInfo, InputStream content, BlobWriteOption... options) throws IOException;

The differences between them:

create() use direct upload, upload() use resumable upload. Would it make sense to reflect this
upload() throw IOException, create() throw StorageException

Would it make sense to use create instead of upload to reflect this?

# Conflicts: # google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplMockitoTest.java # pom.xml

# Conflicts: # google-cloud-storage/clirr-ignored-differences.xml # google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java # google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplMockitoTest.java # google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

frankyn · 2020-06-25T15:10:25Z

@dmitry-fa could you address the conflict added after merging #138 PR?

dmitry-fa · 2020-06-25T15:44:08Z

@frankyn I think it's not needed to be merged.

feat: add storage.upload(path)

6af9ce2

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

dmitry-fa requested review from frankyn and elharo and removed request for frankyn April 17, 2020 14:08

elharo reviewed Apr 17, 2020

View reviewed changes

frankyn requested a review from JesseLovelace April 17, 2020 18:18

frankyn suggested changes Apr 17, 2020

View reviewed changes

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Apr 21, 2020

dmitry-fa force-pushed the storageUpload3 branch from 67c6544 to 6af9ce2 Compare April 21, 2020 14:00

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 21, 2020

dmitry-fa added 4 commits April 21, 2020 17:14

feat: use mockito framework

085739f

feat: update upload to return blob

83ad702

feat: do not parse response from writers for signed urls

94d2948

Merge branch 'master' into storageUpload3

e59bece

# Conflicts: # pom.xml

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020

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

frankyn suggested changes Apr 28, 2020

View reviewed changes

dmitry-fa added 2 commits April 29, 2020 22:18

Merge branch 'master' into storageUpload3

0806031

# Conflicts: # google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplMockitoTest.java # pom.xml

fix reviewer's comments

67a701e

dmitry-fa added 2 commits June 18, 2020 12:48

Update Storage.upload() functionality after merge

d62e615

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

dmitry-fa added 5 commits June 18, 2020 14:33

feat: deprecate StorageRpc.write(), introduce StorageRpc.upload()

d55513f

feat: deprecate StorageRpc.write(), introduce StorageRpc.upload()

773d587

Merge branch 'master' into storageUpload3

0d9b17f

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Merge branch 'storageUpload3_next' into storageUpload3

3c9f2dc

rename upload to createFrom

d7c7f1b

dmitry-fa requested a review from frankyn June 19, 2020 07:58

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2020

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

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 20, 2020

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

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 22, 2020

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

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2020

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

frankyn mentioned this pull request Jun 24, 2020

chore: release 0.130.0 googleapis/java-cloud-bom#665

Merged

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 24, 2020

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

dmitry-fa added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 24, 2020

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

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 25, 2020

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

frankyn approved these changes Jun 25, 2020

View reviewed changes

frankyn merged commit 9457f3a into googleapis:master Jun 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add storage.upload(path) #269

feat: add storage.upload(path) #269

dmitry-fa commented Apr 17, 2020

codecov bot commented Apr 17, 2020 •

edited

Loading

elharo Apr 17, 2020

dmitry-fa Apr 17, 2020

dmitry-fa Apr 21, 2020

frankyn left a comment

frankyn Apr 17, 2020

dmitry-fa Apr 17, 2020

dmitry-fa Apr 17, 2020

frankyn Apr 17, 2020

dmitry-fa Apr 18, 2020 •

edited

Loading

dmitry-fa Apr 20, 2020

dmitry-fa Apr 27, 2020

frankyn Apr 17, 2020

googlebot commented Apr 21, 2020

googlebot commented Apr 21, 2020

frankyn left a comment

frankyn Apr 28, 2020

dmitry-fa Apr 28, 2020

dmitry-fa commented Apr 28, 2020

frankyn commented Apr 28, 2020

dmitry-fa commented Apr 29, 2020

frankyn commented Jun 25, 2020

dmitry-fa commented Jun 25, 2020

feat: add storage.upload(path) #269

feat: add storage.upload(path) #269

Conversation

dmitry-fa commented Apr 17, 2020

codecov bot commented Apr 17, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-fa Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Apr 21, 2020

googlebot commented Apr 21, 2020

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-fa commented Apr 28, 2020

frankyn commented Apr 28, 2020

dmitry-fa commented Apr 29, 2020

frankyn commented Jun 25, 2020

dmitry-fa commented Jun 25, 2020

codecov bot commented Apr 17, 2020 •

edited

Loading

dmitry-fa Apr 18, 2020 •

edited

Loading