Add abort() to writer

[frankyn]

Feature request filed in google-cloud-java: googleapis/google-cloud-java#1232

[dmitry-fa]

Hi @frankyn.

I'd like to suggest an approach for resumable upload implementation. Before proper documenting and writing tests, I'd like to check with you if my approach is applicable.

• extend com.google.cloud.storage.spi.v1.StorageRpc interface with

  /**
   * Aborts the given resumable upload.
   *
   * @throws StorageException upon failure
   */
  void abort(String uploadId);

to send DELETE request on the given uploadId as suggested Issue-1232

• add new method to package private class com.google.cloud.storage.BlobWriteChannel like:

void abort() {
    getOptions().getStorageRpcV1() .abort(getUploadId());
}

• extend com.google.cloud.storage.Storage interface with:

  /**
   * Aborts resumeable upload. Further attempts to write to or close the channel will result in
   * StorageException with the code 499.
   *
   * @param channel an instance crated either by {@link #writer(BlobInfo, BlobWriteOption...)}
   *                or by {@link #writer(URL)} method.
   * @throws IllegalArgumentException if inappropriate channel is given
   * @throws StorageException upon failure
   */
  public void abort(WriteChannel channel) {
    if (!(channel instanceof BlobWriteChannel)) {
      throw new IllegalArgumentException("channel is not created by Storage");
    }
    BlobWriteChannel blobWriteChannel = (BlobWriteChannel)channel;
    blobWriteChannel.abort();
  }

[frankyn]

I wanted abort to be part of WriteChannel interface but it may not make sense for all libraries that support this interface.

Could you review this open question?

Changes to StorageRpc do look okay, but would make the name specific to resumable upload sessions.

// com.google.cloud.storage.spi.v1.StorageRpc
void abortResumableUpload(String uploadId)

[dmitry-fa]

@frankyn,

I wanted abort to be part of WriteChannel interface but it may not make sense for all libraries that support this interface.

I thought WriteChannel is a too generic interface to contain abort() method and didn't plan to extend it.
After your words, I reviewed the spec and found interesting facts:

• WriteChannel implements Closable
• Implementations of this class may further buffer data internally to reduce remote calls. Written data might not be visible until calling Channel.close().

So adding abort() will make sense to tell close() that it's not needed anymore to flush buffered data when close() method is called on the channel.

[frankyn]

I think you'll need to submit a proposal to the interface to see if it's acceptable to add abort(). Could you follow-up on that?

[dmitry-fa]

@frankyn Yes, I will work on this.

[dmitry-fa]

@frankyn, this is the proposal: googleapis/google-cloud-java#9101

[dmitry-fa]

Thoughts and suggestions:

• Why abort is required?
  Aborting is required when the user decides in the middle of uploading that the blob should not be updated/created. Now when an instance of WriteChannel for a blob is created the close() method will be called eventually on the instance, this will cause the blob update/create. The server side already supports DELETE command on the upload URL, but this command is currently not available in the Java client.

• How abort functionality could be added to the Java client?
  The StorageRpc interface should be extended with a new method like void abort(String uploadId). Then an abort() method should be added to the BlobWriteChannel class. The current problem is BlobWriteChannel has a package-private access and we need to decide how to make this method visible for users. Possible suggestions with pros/cons:

  • Extend global WriteChannel interface as suggested in feat: Add abort() to WriteChannel interface google-cloud-java#9101
    +: seems the most logical solution
    -: extending interface is an incompatible change
    -: might take a while to get approval/implement
    -: could be not needed for other extensions of WriteChannel
  • Make BlobWriteChannel/BlobReaderChannel public
    +: doesn't break API
    -: exposes extra API
  • Temporary solution: declare Storage.abort(WriteChannel) method (as proposed in feat: Add abort() to writer #460)
    +: doesn't break API
    +: could be implemented quickly
    -: extends overloaded Storage interface