[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039: URL: https://github.com/apache/hadoop/pull/4039#discussion_r1139125610 ## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java: ## @@ -25,6 +25,7 @@ import java.net.UnknownHostException; import java.util.List; +import org.apache.hadoop.classification.VisibleForTesting; Review Comment: The import is already present in the class after merge with trunk, removed the redundant import. -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1138613208
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java:
##
@@ -229,14 +232,29 @@ private void completeExecute(TracingContext
tracingContext)
}
}
-if (result.getStatusCode() >= HttpURLConnection.HTTP_BAD_REQUEST) {
+int status = result.getStatusCode();
+/*
+ If even after exhausting all retries, the http status code has an
+ invalid value it qualifies for InvalidAbfsRestOperationException.
+ All http status code less than 1xx range are considered as invalid
+ status codes.
+ */
+if (status < HTTP_CONTINUE) {
+ throw new InvalidAbfsRestOperationException(null, retryCount);
Review Comment:
Hardcoded while calling the constructor of the parent since the status code
for invalid abfsrestoperation is always -1.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1138611457
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/InvalidAbfsRestOperationException.java:
##
@@ -30,14 +30,33 @@
@InterfaceAudience.Public
@InterfaceStability.Evolving
public class InvalidAbfsRestOperationException extends
AbfsRestOperationException {
+
+ private static final String ERROR_MESSAGE =
"InvalidAbfsRestOperationException";
+
public InvalidAbfsRestOperationException(
final Exception innerException) {
super(
AzureServiceErrorCode.UNKNOWN.getStatusCode(),
AzureServiceErrorCode.UNKNOWN.getErrorCode(),
innerException != null
? innerException.toString()
-: "InvalidAbfsRestOperationException",
+: ERROR_MESSAGE,
innerException);
}
+
+ /**
+ * Adds the retry count along with the exception.
+ * @param innerException The inner exception which is originally caught.
+ * @param retryCount The retry count when the exception was thrown.
+ */
+ public InvalidAbfsRestOperationException(
+ final Exception innerException, int retryCount) {
Review Comment:
This exception is only thrown for invalid case so by default the constructor
takes the status code as -1.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1057110694
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##
@@ -681,35 +685,36 @@ public AbfsRestOperation append(final String path, final
byte[] buffer,
abfsUriQueryBuilder, cachedSasToken);
final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString());
-final AbfsRestOperation op = new AbfsRestOperation(
-AbfsRestOperationType.Append,
-this,
-HTTP_METHOD_PUT,
-url,
-requestHeaders,
-buffer,
-reqParams.getoffset(),
-reqParams.getLength(),
-sasTokenForReuse);
+final AbfsRestOperation op =
getAbfsRestOperationForAppend(AbfsRestOperationType.Append,
+HTTP_METHOD_PUT, url, requestHeaders, buffer, reqParams.getoffset(),
+reqParams.getLength(), sasTokenForReuse);
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.
Review Comment:
Today in some places, devs have taken dependency on the response message
being sent by the server for the further processing in code. In case if server
doesn't send a response of 100-continue, client has not sent the payload and
hence the response message may be null in some cases. So to make sure that
backward compatibility is not hindered, we are retrying the request failed with
user errors without expect header being enabled.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1057109478
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##
@@ -314,13 +327,46 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
if (this.isTraceEnabled) {
startTime = System.nanoTime();
}
-try (OutputStream outputStream = this.connection.getOutputStream()) {
- // update bytes sent before they are sent so we may observe
- // attempted sends as well as successful sends via the
- // accompanying statusCode
+OutputStream outputStream = null;
+// Updates the expected bytes to be sent based on length.
+this.expectedBytesToBeSent = length;
+try {
+ try {
+/* Without expect header enabled, if getOutputStream() throws
+ an exception, it gets caught by the restOperation. But with
+ expect header enabled we return back without throwing an exception
+ for the correct response code processing.
+ */
+outputStream = this.connection.getOutputStream();
+ } catch (IOException e) {
+/* If getOutputStream fails with an exception and expect header
+ is enabled, we return back without throwing an exception to
+ the caller. The caller is responsible for setting the correct
status code.
+ If expect header is not enabled, we throw back the exception.
+ */
+String expectHeader = this.connection.getRequestProperty(EXPECT);
+if (expectHeader != null && expectHeader.equals(HUNDRED_CONTINUE)) {
+ LOG.debug("Getting output stream failed with expect header enabled,
returning back "
+ + ExceptionUtils.getStackTrace(e));
+ return;
Review Comment:
So, the response code will depend on getOutputStream failed due to which
reason, if it fails due to openSSL Handshake and expect header is enabled, it
will get the status of 0 or -1 in the processResponse being called in
AbfsHttpOperation and thus the request will get retried. The status code of
>400 is not guaranteed, the correct understanding of what the response code is
can be done by the getResponseCode() in the processResponse only. So would not
be right to throw and exception from this point. The flow should be from
AbfsRestOperation to client only.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1055400588
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##
@@ -681,35 +685,36 @@ public AbfsRestOperation append(final String path, final
byte[] buffer,
abfsUriQueryBuilder, cachedSasToken);
final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString());
-final AbfsRestOperation op = new AbfsRestOperation(
-AbfsRestOperationType.Append,
-this,
-HTTP_METHOD_PUT,
-url,
-requestHeaders,
-buffer,
-reqParams.getoffset(),
-reqParams.getLength(),
-sasTokenForReuse);
+final AbfsRestOperation op =
getAbfsRestOperationForAppend(AbfsRestOperationType.Append,
+HTTP_METHOD_PUT, url, requestHeaders, buffer, reqParams.getoffset(),
+reqParams.getLength(), sasTokenForReuse);
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.
Review Comment:
There is no rfc that specifies this behaviour but in case of expect failing
with any non http 100 response, the response message is set to null. That's why
we mentioned that it might fail to provide all response headers.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1055399560
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java:
##
@@ -404,4 +431,152 @@ public static AbfsRestOperation
getRestOp(AbfsRestOperationType type,
public static AccessTokenProvider getAccessTokenProvider(AbfsClient client) {
return client.getTokenProvider();
}
+
+ /**
+ * Test helper method to get random bytes array.
+ * @param length The length of byte buffer.
+ * @return byte buffer.
+ */
+ private byte[] getRandomBytesArray(int length) {
+final byte[] b = new byte[length];
+new Random().nextBytes(b);
+return b;
+ }
+
+ /**
+ * Test to verify that client retries append request without
+ * expect header enabled if append with expect header enabled fails
+ * with 4xx kind of error.
+ * @throws Exception
+ */
+ @Test
+ public void testExpectHundredContinue() throws Exception {
+// Get the filesystem.
+final AzureBlobFileSystem fs = getFileSystem();
+
+final Configuration configuration = new Configuration();
+configuration.addResource(TEST_CONFIGURATION_FILE_NAME);
+AbfsClient abfsClient = fs.getAbfsStore().getClient();
+
+AbfsConfiguration abfsConfiguration = new AbfsConfiguration(configuration,
+configuration.get(FS_AZURE_ABFS_ACCOUNT_NAME));
+
+// Update the configuration with reduced retry count and reduced backoff
interval.
+AbfsConfiguration abfsConfig
+= TestAbfsConfigurationFieldsValidation.updateRetryConfigs(
+abfsConfiguration,
+REDUCED_RETRY_COUNT, REDUCED_BACKOFF_INTERVAL);
+
+// Gets the client.
+AbfsClient testClient = Mockito.spy(
+TestAbfsClient.createTestClientFromCurrentContext(
+abfsClient,
+abfsConfig));
+
+// Create the append request params with expect header enabled initially.
+AppendRequestParameters appendRequestParameters
+= new AppendRequestParameters(
+BUFFER_OFFSET, BUFFER_OFFSET, BUFFER_LENGTH,
+AppendRequestParameters.Mode.APPEND_MODE, false, null, true);
+
+byte[] buffer = getRandomBytesArray(BUFFER_LENGTH);
+
+// Create a test container to upload the data.
+Path testPath = path(TEST_PATH);
+fs.create(testPath);
+String finalTestPath = testPath.toString()
+.substring(testPath.toString().lastIndexOf("/"));
+
+// Creates a list of request headers.
+final List requestHeaders
+= TestAbfsClient.getTestRequestHeaders(testClient);
+requestHeaders.add(
+new AbfsHttpHeader(X_HTTP_METHOD_OVERRIDE, HTTP_METHOD_PATCH));
+if (appendRequestParameters.isExpectHeaderEnabled()) {
+ requestHeaders.add(new AbfsHttpHeader(EXPECT, HUNDRED_CONTINUE));
+}
+
+// Updates the query parameters.
+final AbfsUriQueryBuilder abfsUriQueryBuilder
+= testClient.createDefaultUriQueryBuilder();
+abfsUriQueryBuilder.addQuery(QUERY_PARAM_ACTION, APPEND_ACTION);
+abfsUriQueryBuilder.addQuery(QUERY_PARAM_POSITION,
+Long.toString(appendRequestParameters.getPosition()));
+
+// Creates the url for the specified path.
+URL url = testClient.createRequestUrl(finalTestPath,
abfsUriQueryBuilder.toString());
+
+// Create a mock of the AbfsRestOperation to set the urlConnection in the
corresponding httpOperation.
+AbfsRestOperation op = Mockito.spy(new AbfsRestOperation(
+AbfsRestOperationType.Append,
+testClient,
+HTTP_METHOD_PUT,
+url,
+requestHeaders, buffer,
+appendRequestParameters.getoffset(),
+appendRequestParameters.getLength(), null));
+
+AbfsHttpOperation abfsHttpOperation = new AbfsHttpOperation(url,
+HTTP_METHOD_PUT, requestHeaders);
+
+// Create a mock of UrlConnection class.
+HttpURLConnection urlConnection = mock(HttpURLConnection.class);
+
+// Sets the expect request property if expect header is enabled.
+if (appendRequestParameters.isExpectHeaderEnabled()) {
+ Mockito.doReturn(HUNDRED_CONTINUE).when(urlConnection)
+ .getRequestProperty(EXPECT);
+}
+Mockito.doNothing().when(urlConnection).setRequestProperty(Mockito
+.any(), Mockito.any());
+Mockito.doReturn(url).when(urlConnection).getURL();
+
+// Give user error code 404 when processResponse is called.
+Mockito.doReturn(HTTP_METHOD_PUT).when(urlConnection).getRequestMethod();
+Mockito.doReturn(HTTP_NOT_FOUND).when(urlConnection).getResponseCode();
+Mockito.doReturn("Resource Not Found")
+.when(urlConnection)
+.getResponseMessage();
+
+// Make the getOutputStream throw IOException to see it returns from the
sendRequest correctly.
+Mockito.doThrow(new ProtocolException("Server rejected Operation"))
+.when(urlConnection)
+.getOutputStream();
+abfsHttpOperation.setConnection(urlConnection);
Review
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1055218825
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##
@@ -314,13 +327,46 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
if (this.isTraceEnabled) {
startTime = System.nanoTime();
}
-try (OutputStream outputStream = this.connection.getOutputStream()) {
- // update bytes sent before they are sent so we may observe
- // attempted sends as well as successful sends via the
- // accompanying statusCode
+OutputStream outputStream = null;
+// Updates the expected bytes to be sent based on length.
+this.expectedBytesToBeSent = length;
+try {
+ try {
+/* Without expect header enabled, if getOutputStream() throws
+ an exception, it gets caught by the restOperation. But with
+ expect header enabled we return back without throwing an exception
+ for the correct response code processing.
+ */
+outputStream = this.connection.getOutputStream();
+ } catch (IOException e) {
+/* If getOutputStream fails with an exception and expect header
+ is enabled, we return back without throwing an exception to
+ the caller. The caller is responsible for setting the correct
status code.
+ If expect header is not enabled, we throw back the exception.
+ */
+String expectHeader = this.connection.getRequestProperty(EXPECT);
+if (expectHeader != null && expectHeader.equals(HUNDRED_CONTINUE)) {
+ LOG.debug("Getting output stream failed with expect header enabled,
returning back "
+ + ExceptionUtils.getStackTrace(e));
+ return;
Review Comment:
The request was sent to the server with expect header enabled but it didn't
respond back with 100-continue because client didn't meet with the
expectations. So the connection was successfully set and hence it can set all
these properties accordingly.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1055138094
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/InvalidAbfsRestOperationException.java:
##
@@ -30,14 +30,33 @@
@InterfaceAudience.Public
@InterfaceStability.Evolving
public class InvalidAbfsRestOperationException extends
AbfsRestOperationException {
+
+ private static final String ERROR_MESSAGE =
"InvalidAbfsRestOperationException";
+
public InvalidAbfsRestOperationException(
final Exception innerException) {
super(
AzureServiceErrorCode.UNKNOWN.getStatusCode(),
AzureServiceErrorCode.UNKNOWN.getErrorCode(),
innerException != null
? innerException.toString()
-: "InvalidAbfsRestOperationException",
+: ERROR_MESSAGE,
Review Comment:
Adding it as a string so that it can be modified as per convenience.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1040936094
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##
@@ -314,18 +314,21 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
if (this.isTraceEnabled) {
startTime = System.nanoTime();
}
-try (OutputStream outputStream = this.connection.getOutputStream()) {
- // update bytes sent before they are sent so we may observe
- // attempted sends as well as successful sends via the
- // accompanying statusCode
- this.bytesSent = length;
+OutputStream outputStream;
+try {
+ try {
+outputStream = this.connection.getOutputStream();
+ } catch (IOException e) {
+// If getOutputStream fails with an exception due to 100-continue
Review Comment:
1. The first point is valid, I have made the change where getOutputStream
throws exception for the cases where 100 continue is not enabled and returns
back to the caller when it catches an IOException due to 100 continue being
enabled which leads to processResponse getting the correct status code and then
eventually the retry logic coming into play.
2. We need to update the bytes sent for failed as well as passed cases. The
current change will not swallow any exceptions.
The handling for various status code with 100 continue enabled is as follows
1. Case 1 :- getOutputStream doesn't throw any exception, response is
processed and it gives status code of 200, no retry is needed and hence the
request succeeds.
2. Case 2:- getOutputSteam throws exception, we return to the caller and
in processResponse in this.connection.getResponseCode() it gives status code of
404 (user error), exponential retry is not needed. We retry without 100
continue enabled.
3. Case 3:- getOutputSteam throws exception, we return to the caller and
in processResponse it gives status code of 503,
which shows throttling so we backoff accordingly with exponential retry.
Since each append request waits for 100 continue response, the stress on the
server gets reduced.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1040936094
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##
@@ -314,18 +314,21 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
if (this.isTraceEnabled) {
startTime = System.nanoTime();
}
-try (OutputStream outputStream = this.connection.getOutputStream()) {
- // update bytes sent before they are sent so we may observe
- // attempted sends as well as successful sends via the
- // accompanying statusCode
- this.bytesSent = length;
+OutputStream outputStream;
+try {
+ try {
+outputStream = this.connection.getOutputStream();
+ } catch (IOException e) {
+// If getOutputStream fails with an exception due to 100-continue
Review Comment:
1. The first point is valid, I have made the change where getOutputStream
throws exception for the cases where 100 continue is not enabled and returns
back to the caller when it catches an IOException due to 100 continue being
enabled which leads to processResponse getting the correct status code and then
eventually the retry logic coming into play.
2. We need to update the bytes sent for failed as well as passed cases. The
current change will not swallow any exceptions.
The handling for various status code with 100 continue enabled is as follows
1. Case 1 :- getOutputStream doesn't throw any exception, response is
processed and it gives status code of 200, no retry is needed and hence the
request succeeds.
2. Case 2:- getOutputSteam throws exception, we return to the caller and in
processResponse in this.connection.getResponseCode() it gives status code of
404 (user error), exponential retry is not needed. We retry without 100
continue enabled.
3. Case 3:- getOutputSteam throws exception, we return to the caller and in
processResponse it gives status code of 503,
which shows throttling so we backoff accordingly with exponential retry.
Since each append request waits for 100 continue response, the stress on the
server gets reduced.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039: URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007964037 ## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ## @@ -652,6 +653,9 @@ public AbfsRestOperation append(final String path, final byte[] buffer, addCustomerProvidedKeyHeaders(requestHeaders); // JDK7 does not support PATCH, so to workaround the issue we will use Review Comment: Done. -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007963664
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/HttpHeaderConfigurations.java:
##
@@ -60,6 +60,7 @@ public final class HttpHeaderConfigurations {
public static final String X_MS_UMASK = "x-ms-umask";
public static final String X_MS_NAMESPACE_ENABLED = "x-ms-namespace-enabled";
public static final String X_MS_ABFS_CLIENT_LATENCY =
"x-ms-abfs-client-latency";
+ public static final String EXPECT = "Expect";
Review Comment:
Done.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007963420
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java:
##
@@ -694,6 +694,7 @@ private AbfsOutputStreamContext
populateAbfsOutputStreamContext(
return new
AbfsOutputStreamContext(abfsConfiguration.getSasTokenRenewPeriodForStreamsInSeconds())
.withWriteBufferSize(bufferSize)
.enableFlush(abfsConfiguration.isFlushEnabled())
+.enableExpectHeader(abfsConfiguration.isExpectHeaderEnabled())
Review Comment:
Done.
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java:
##
@@ -103,6 +103,7 @@ public final class AbfsHttpConstants {
public static final String DEFAULT_SCOPE = "default:";
public static final String PERMISSION_FORMAT = "%04d";
public static final String SUPER_USER = "$superuser";
+ public static final String HUNDRED_CONTINUE = "100-continue";
Review Comment:
Added comments.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007963256
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -22,10 +22,14 @@
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MAX_BACKOFF_INTERVAL;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MAX_IO_RETRIES;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MIN_BACKOFF_INTERVAL;
+
import java.util.Random;
+
import org.junit.Assert;
import org.junit.Test;
+
import org.apache.hadoop.conf.Configuration;
+
Review Comment:
This line break is in trunk as well.
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##
@@ -730,6 +729,18 @@ && appendSuccessCheckOp(op, path,
return op;
}
+ /**
+ * Returns true if the status code lies in the range of user error
+ * @param e Exception caught
+ * @return True or False
+ */
+ private boolean checkUserError(AzureBlobFileSystemException e) {
+return ((AbfsRestOperationException) e).getStatusCode()
Review Comment:
Done.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007962889
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##
@@ -688,6 +692,17 @@ 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.
Review Comment:
Made the changes.
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/services/AppendRequestParameters.java:
##
@@ -34,19 +34,21 @@ public enum Mode {
private final Mode mode;
private final boolean isAppendBlob;
private final String leaseId;
+ private boolean isExpectHeaderEnabled;
public AppendRequestParameters(final long position,
final int offset,
final int length,
final Mode mode,
final boolean isAppendBlob,
- final String leaseId) {
+ final String leaseId, boolean isExpectHeaderEnabled) {
Review Comment:
Done.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007962664
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##
@@ -320,6 +320,8 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
// accompanying statusCode
this.bytesSent = length;
outputStream.write(buffer, offset, length);
+} catch (IOException e) {
+ this.bytesSent = length;
Review Comment:
Added comments
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] anmolanmol1234 commented on a diff in pull request #4039: HADOOP-18146: ABFS: Added changes for expect hundred continue header
anmolanmol1234 commented on code in PR #4039:
URL: https://github.com/apache/hadoop/pull/4039#discussion_r1007939204
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java:
##
@@ -32,7 +32,7 @@
public final class FileSystemConfigurations {
public static final String DEFAULT_FS_AZURE_ACCOUNT_IS_HNS_ENABLED = "";
-
+ public static final boolean
DEFAULT_FS_AZURE_ACCOUNT_IS_EXPECT_HEADER_ENABLED = true;
Review Comment:
We tested by enabling the header for low as well as high throttled workloads
and it gives better performance in both situations, hence we add it by default
with each append request.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
