[GitHub] [hadoop] HeartSaVioR commented on a change in pull request #2667: WIP. HADOOP-16906. Add Abortable.abort() interface for streams to enable output stream to be terminated

[GitBox <gi...@apache.org>]

HeartSaVioR commented on a change in pull request #2667:
URL: https://github.com/apache/hadoop/pull/2667#discussion_r567685195



##########
File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java
##########
@@ -551,6 +556,24 @@ public IOStatistics getIOStatistics() {
     return iostatistics;
   }
 
+  @Override
+  public void abort() {

Review comment:
       Migrating comment in https://github.com/HeartSaVioR/hadoop/commit/63c5588d46d422a66ec620ae310f9a860db890b3#r46507150
   
   > the new "cloud ready" API calls always return a CompletableFuture, to emphasise that the op may take time and to allow the caller to do something while waiting. Would we want to do this here? I'm not convinced it is appropriate. Instead we say
   >
   > 1. call must guarantee that after this is invoked,. close() will not materialize the file at its final path
   > 2. it may communicate with the store to cancel an operation; which may retry. Errors will be stored.
   > 3. there may still/also be async IO to the store after the call returns, but this must maintain the requirement "not visible"
   > 4. And close() may do some IO to cancel

##########
File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java
##########
@@ -551,6 +556,24 @@ public IOStatistics getIOStatistics() {
     return iostatistics;
   }
 
+  @Override
+  public void abort() {
+    if (closed.getAndSet(true)) {
+      // already closed
+      LOG.debug("Ignoring abort() as stream is already closed");
+      return;
+    }
+
+    S3ADataBlocks.DataBlock block = getActiveBlock();

Review comment:
       Migrating comment in https://github.com/HeartSaVioR/hadoop/commit/63c5588d46d422a66ec620ae310f9a860db890b3#r46506593
   
   > I think we are going to have to worry about this a bit more, because we may have queued >1 block for upload in a separate thread. They'll maybe need interruption, or at least, when they finish, see if they should immediately cancel the upload. This won't make any difference in the semantics of abort() (the final upload has been killed), I just don't want to run up any bills.

##########
File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java
##########
@@ -551,6 +556,24 @@ public IOStatistics getIOStatistics() {
     return iostatistics;
   }
 
+  @Override
+  public void abort() {
+    if (closed.getAndSet(true)) {
+      // already closed
+      LOG.debug("Ignoring abort() as stream is already closed");
+      return;
+    }
+
+    S3ADataBlocks.DataBlock block = getActiveBlock();
+    try {
+      if (multiPartUpload != null) {
+        multiPartUpload.abort();

Review comment:
       Migrating comment in https://github.com/HeartSaVioR/hadoop/commit/63c5588d46d422a66ec620ae310f9a860db890b3#r46506765
   
   > ok, don't worry so much about my prev comment. That cancels all the outstanding futures.

##########
File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3ABlockOutputStream.java
##########
@@ -82,4 +82,22 @@ public void testWriteOperationHelperPartLimits() throws Throwable {
         () -> woh.newUploadPartRequest(key,
             "uploadId", 50000, 1024, inputStream, null, 0L));
   }
+
+  @Test
+  public void testStreamClosedAfterAbort() throws Exception {
+    stream.abort();
+
+    // This verification replaces testing various operations after calling abort:
+    // after calling abort, stream is closed like calling close().
+    intercept(IOException.class, () -> stream.checkOpen());

Review comment:
       Migrating comment in https://github.com/HeartSaVioR/hadoop/commit/63c5588d46d422a66ec620ae310f9a860db890b3#r46507220
   
   > should also verify that stream.write() raises an IOE. We could raise a subclass of IOE to indicate this was a checkOpen failure for a stricter test

##########
File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3ABlockOutputStream.java
##########
@@ -82,4 +82,22 @@ public void testWriteOperationHelperPartLimits() throws Throwable {
         () -> woh.newUploadPartRequest(key,
             "uploadId", 50000, 1024, inputStream, null, 0L));
   }
+
+  @Test

Review comment:
       Migrating comment in https://github.com/HeartSaVioR/hadoop/commit/63c5588d46d422a66ec620ae310f9a860db890b3#r46506646
   
   > tests are good. We will need to do an ITest too, which can be done in ITestS3ABlockOutputArray




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org