[GitHub] [hadoop] snvijaya commented on a diff in pull request #6022: Expect100resolution

["snvijaya (via GitHub)" <gi...@apache.org>]

snvijaya commented on code in PR #6022:
URL: https://github.com/apache/hadoop/pull/6022#discussion_r1317128267


##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##########
@@ -391,6 +398,23 @@ public void processResponse(final byte[] buffer, final int offset, final int len
 
     this.statusDescription = getConnResponseMessage();
 
+    /*

Review Comment:
   Wont above getResponseCode() and getResponseMessage also lead to connections being established ? If yes, we should probably return right at the start of the method itself if connectionDisconnectedOnError is true.



##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java:
##########
@@ -69,6 +69,7 @@ public final class AbfsHttpConstants {
    * and should qualify for retry.
    */
   public static final int HTTP_CONTINUE = 100;
+  public static final String EXPECT_100_JDK_ERROR = "Server rejected operation";

Review Comment:
   Wont the JDK bug were redundant connections are attempted be an issue for any exception case in getOutputStream() ? Shouldn't the case to handle be irrespective of explicit server rejected request case ?



##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##########
@@ -340,8 +344,11 @@ public void sendRequest(byte[] buffer, int offset, int length) throws IOExceptio
            If expect header is not enabled, we throw back the exception.
          */
         String expectHeader = getConnProperty(EXPECT);
-        if (expectHeader != null && expectHeader.equals(HUNDRED_CONTINUE)) {
+        if (expectHeader != null && expectHeader.equals(HUNDRED_CONTINUE)
+            && e instanceof ProtocolException
+            && EXPECT_100_JDK_ERROR.equals(e.getMessage())) {

Review Comment:
   As mentioned in an earlier comment, the HttpUrlConnection that we hold currently is disconnected while inside this catch block. 
   
   Do we want to prevent later API calls that trigger connections irrespective of throttled failures ? If so, setting the status of connectionDisconnectedOnError should be at the start of catch block ?



##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##########
@@ -85,6 +88,7 @@ public class AbfsHttpOperation implements AbfsPerfLoggable {
   private long sendRequestTimeMs;
   private long recvResponseTimeMs;
   private boolean shouldMask = false;
+  private boolean expect100failureReceived = false;

Review Comment:
   Though the significance of the bug is seen in 100-Continue flow, the state of the previously established connection is the reason to prevent getHeaderField* APIs. Would suggest renaming this field to connectionDisconnectedOnError and also add a code comment that this is a workaround for JDK bug with link. 



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