[GitHub] [hadoop] steveloughran commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header

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

steveloughran commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1029705420


##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/services/AppendRequestParameters.java:
##########
@@ -72,4 +75,12 @@ public boolean isAppendBlob() {
   public String getLeaseId() {
     return this.leaseId;
   }
+
+  public boolean getIsExpectHeaderEnabled() {

Review Comment:
   change to `isExpectHeaderEnabled()` for consistency with the AbfsOutputStreamContext property



##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -688,6 +692,19 @@ public AbfsRestOperation append(final String path, final byte[] buffer,
     try {
       op.execute(tracingContext);
     } catch (AzureBlobFileSystemException e) {
+      // If the http response code indicates a user error we retry
+      // the same append request with expect header disabled.
+      // When "100-continue" header is enabled but a non Http 100 response comes,
+      // JDK fails to provide all response headers.
+      // This handling is to avoid breaking of backward compatibility
+      // if someone has taken dependency on the exception message,
+      // which is created using the error string present in the response header.
+      int responseStatusCode = ((AbfsRestOperationException) e).getStatusCode();
+      if (checkUserError(responseStatusCode) && reqParams.getIsExpectHeaderEnabled()) {
+        reqParams.setExpectHeaderEnabled(false);

Review Comment:
   how about logging this at debug so if it happens, there's more of a history people can look at



##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##########
@@ -320,6 +320,11 @@ public void sendRequest(byte[] buffer, int offset, int length) throws IOExceptio
       // accompanying statusCode
       this.bytesSent = length;
       outputStream.write(buffer, offset, length);
+    } catch (IOException e) {
+      // If getOutputStream fails with an exception due to 100-continue
+      // enabled, we update the bytes sent before they are sent
+      // in the catch block.
+      this.bytesSent = length;

Review Comment:
   this is going to swallow all IOEs raised. unless i've misreasd something, the IOE *must* be rethrown



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

To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org

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