[ 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: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org