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