Fixes #179

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]

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

[dmitry-fa]

@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.

[dmitry-fa]

@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.

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

[frankyn]

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

[dmitry-fa]

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.

[dmitry-fa]

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

[frankyn]

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.

[dmitry-fa]

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

[dmitry-fa]

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:

1. 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.

2. 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
         }
}

3. 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?

[dmitry-fa]

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

[frankyn]

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.

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.

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

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]

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

[dmitry-fa]

Agree. Done #284

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.

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.

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?

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

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