[
https://issues.apache.org/jira/browse/HADOOP-17890?focusedWorklogId=765218&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-765218
]
ASF GitHub Bot logged work on HADOOP-17890:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 02/May/22 23:49
Start Date: 02/May/22 23:49
Worklog Time Spent: 10m
Work Description: raymondlam12 commented on code in PR #3381:
URL: https://github.com/apache/hadoop/pull/3381#discussion_r863255244
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##########
@@ -336,95 +347,141 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
*
* @throws IOException if an error occurs.
*/
- public void processResponse(final byte[] buffer, final int offset, final int
length) throws IOException {
+ public void processResponse(final byte[] buffer,
+ final int offset,
+ final int length) throws IOException {
// get the response
long startTime = 0;
- if (this.isTraceEnabled) {
+ if (isTraceEnabled) {
startTime = System.nanoTime();
}
- this.statusCode = this.connection.getResponseCode();
+ statusCode = connection.getResponseCode();
- if (this.isTraceEnabled) {
- this.recvResponseTimeMs = elapsedTimeMs(startTime);
+ if (isTraceEnabled) {
+ recvResponseTimeMs = elapsedTimeMs(startTime);
}
- this.statusDescription = this.connection.getResponseMessage();
+ statusDescription = connection.getResponseMessage();
- this.requestId =
this.connection.getHeaderField(HttpHeaderConfigurations.X_MS_REQUEST_ID);
- if (this.requestId == null) {
- this.requestId = AbfsHttpConstants.EMPTY_STRING;
+ requestId =
connection.getHeaderField(HttpHeaderConfigurations.X_MS_REQUEST_ID);
+ if (requestId == null) {
+ requestId = AbfsHttpConstants.EMPTY_STRING;
}
// dump the headers
AbfsIoUtils.dumpHeadersToDebugLog("Response Headers",
connection.getHeaderFields());
- if (AbfsHttpConstants.HTTP_METHOD_HEAD.equals(this.method)) {
+ if (AbfsHttpConstants.HTTP_METHOD_HEAD.equals(method)) {
// If it is HEAD, and it is ERROR
return;
}
- if (this.isTraceEnabled) {
+ if (isTraceEnabled) {
startTime = System.nanoTime();
}
+ long totalBytesRead = 0;
+
+ try {
+ totalBytesRead = parseResponse(buffer, offset, length);
+ } finally {
+ if (isTraceEnabled) {
+ recvResponseTimeMs += elapsedTimeMs(startTime);
+ }
+ bytesReceived = totalBytesRead;
+ }
+ }
+
+ /**
+ * Detects if the Http response indicates an error or success response.
+ * Parses the response and returns the number of bytes read from the
+ * response.
+ *
+ * @param buffer a buffer to hold the response entity body.
+ * @param offset an offset in the buffer where the data will being.
+ * @param length the number of bytes to be written to the buffer.
+ * @return number of bytes read from response InputStream.
+ * @throws IOException if an error occurs.
+ */
+ public long parseResponse(final byte[] buffer,
+ final int offset,
+ final int length) throws IOException {
if (statusCode >= HttpURLConnection.HTTP_BAD_REQUEST) {
processStorageErrorResponse();
- if (this.isTraceEnabled) {
- this.recvResponseTimeMs += elapsedTimeMs(startTime);
- }
- this.bytesReceived =
this.connection.getHeaderFieldLong(HttpHeaderConfigurations.CONTENT_LENGTH, 0);
+ return connection.getHeaderFieldLong(
+ HttpHeaderConfigurations.CONTENT_LENGTH, 0);
} else {
- // consume the input stream to release resources
- int totalBytesRead = 0;
-
- try (InputStream stream = this.connection.getInputStream()) {
+ try (InputStream stream = connection.getInputStream()) {
if (isNullInputStream(stream)) {
- return;
+ return 0;
}
- boolean endOfStream = false;
- // this is a list operation and need to retrieve the data
- // need a better solution
- if (AbfsHttpConstants.HTTP_METHOD_GET.equals(this.method) && buffer ==
null) {
+ // Incase of ListStatus call, request is of GET Method and the
+ // caller doesnt provide buffer because the length can not be
+ // pre-determined
+ if (AbfsHttpConstants.HTTP_METHOD_GET.equals(method)
+ && buffer == null) {
parseListFilesResponse(stream);
} else {
- if (buffer != null) {
- while (totalBytesRead < length) {
- int bytesRead = stream.read(buffer, offset + totalBytesRead,
length - totalBytesRead);
- if (bytesRead == -1) {
- endOfStream = true;
- break;
- }
- totalBytesRead += bytesRead;
- }
- }
- if (!endOfStream && stream.read() != -1) {
- // read and discard
- int bytesRead = 0;
- byte[] b = new byte[CLEAN_UP_BUFFER_SIZE];
- while ((bytesRead = stream.read(b)) >= 0) {
- totalBytesRead += bytesRead;
- }
- }
+ return readDataFromStream(stream, buffer, offset, length);
}
- } catch (IOException ex) {
- LOG.warn("IO/Network error: {} {}: {}",
- method, getMaskedUrl(), ex.getMessage());
- LOG.debug("IO Error: ", ex);
- throw ex;
- } finally {
- if (this.isTraceEnabled) {
- this.recvResponseTimeMs += elapsedTimeMs(startTime);
+ }
+ }
Review Comment:
Is the catch block that logs the IOException intentionally dropped,
specifically code block [L412,L417]?
If so, why?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java:
##########
@@ -336,95 +347,141 @@ public void sendRequest(byte[] buffer, int offset, int
length) throws IOExceptio
*
* @throws IOException if an error occurs.
*/
- public void processResponse(final byte[] buffer, final int offset, final int
length) throws IOException {
+ public void processResponse(final byte[] buffer,
+ final int offset,
+ final int length) throws IOException {
// get the response
long startTime = 0;
- if (this.isTraceEnabled) {
+ if (isTraceEnabled) {
startTime = System.nanoTime();
}
- this.statusCode = this.connection.getResponseCode();
+ statusCode = connection.getResponseCode();
- if (this.isTraceEnabled) {
- this.recvResponseTimeMs = elapsedTimeMs(startTime);
+ if (isTraceEnabled) {
+ recvResponseTimeMs = elapsedTimeMs(startTime);
}
- this.statusDescription = this.connection.getResponseMessage();
+ statusDescription = connection.getResponseMessage();
- this.requestId =
this.connection.getHeaderField(HttpHeaderConfigurations.X_MS_REQUEST_ID);
- if (this.requestId == null) {
- this.requestId = AbfsHttpConstants.EMPTY_STRING;
+ requestId =
connection.getHeaderField(HttpHeaderConfigurations.X_MS_REQUEST_ID);
+ if (requestId == null) {
+ requestId = AbfsHttpConstants.EMPTY_STRING;
}
// dump the headers
AbfsIoUtils.dumpHeadersToDebugLog("Response Headers",
connection.getHeaderFields());
- if (AbfsHttpConstants.HTTP_METHOD_HEAD.equals(this.method)) {
+ if (AbfsHttpConstants.HTTP_METHOD_HEAD.equals(method)) {
// If it is HEAD, and it is ERROR
return;
}
- if (this.isTraceEnabled) {
+ if (isTraceEnabled) {
startTime = System.nanoTime();
}
+ long totalBytesRead = 0;
Review Comment:
nit: totalBytesRead doesn't look needed here.
This logic can be simplified to:
1. Set bytesReceived initially to 0
2. Assign bytesReceived in the try statement
3. Remove the assignment of bytesReceived in the finally statement
Issue Time Tracking
-------------------
Worklog Id: (was: 765218)
Time Spent: 2h (was: 1h 50m)
> ABFS: Refactor HTTP request handling code
> -----------------------------------------
>
> Key: HADOOP-17890
> URL: https://issues.apache.org/jira/browse/HADOOP-17890
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Affects Versions: 3.4.0
> Reporter: Sneha Vijayarajan
> Assignee: Sneha Vijayarajan
> Priority: Major
> Labels: pull-request-available
> Time Spent: 2h
> Remaining Estimate: 0h
>
> Aims at Http request handling code refactoring.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
