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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
