[ https://issues.apache.org/jira/browse/HADOOP-18425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17702465#comment-17702465 ]
ASF GitHub Bot commented on HADOOP-18425: ----------------------------------------- mukund-thakur commented on code in PR #5494: URL: https://github.com/apache/hadoop/pull/5494#discussion_r1141673602 ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java: ########## @@ -76,6 +76,8 @@ public FilterFileSystem(FileSystem fs) { this.statistics = fs.statistics; } + Review Comment: cut extra lines? ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ########## @@ -613,39 +644,58 @@ private void incrementAbfsRenamePath() { * Exceptions raised in the probe of the destination are swallowed, * so that they do not interfere with the original rename failures. * @param source source path + * @param sourceEtag etag of source file. may be null or empty * @param op Rename request REST operation response with non-null HTTP response * @param destination rename destination path - * @param sourceEtag etag of source file. may be null or empty * @param tracingContext Tracks identifiers for request header + * @param isDir is the source a file or directory * @return true if the file was successfully copied */ public boolean renameIdempotencyCheckOp( final String source, final String sourceEtag, final AbfsRestOperation op, final String destination, - TracingContext tracingContext) { + TracingContext tracingContext, + final boolean isDir) { Preconditions.checkArgument(op.hasResult(), "Operations has null HTTP response"); - if ((op.isARetriedRequest()) - && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) - && isNotEmpty(sourceEtag)) { - + if (!(op.isARetriedRequest()) + && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND)) { + // not an error + return false; + } + LOG.debug("Source not found on retry of rename({}, {}) isDir {} etag {}", + source, destination, isDir, sourceEtag); + if (isDir) { + // directory recovery is not supported. + // log and fail. + LOG.info("rename directory {} to {} failed; unable to recover", + source, destination); + return false; + } + if (isNotEmpty(sourceEtag)) { // Server has returned HTTP 404, which means rename source no longer // exists. Check on destination status and if its etag matches // that of the source, consider it to be a success. Review Comment: these comments not valid here. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ########## @@ -613,39 +644,58 @@ private void incrementAbfsRenamePath() { * Exceptions raised in the probe of the destination are swallowed, * so that they do not interfere with the original rename failures. * @param source source path + * @param sourceEtag etag of source file. may be null or empty * @param op Rename request REST operation response with non-null HTTP response * @param destination rename destination path - * @param sourceEtag etag of source file. may be null or empty * @param tracingContext Tracks identifiers for request header + * @param isDir is the source a file or directory * @return true if the file was successfully copied */ public boolean renameIdempotencyCheckOp( final String source, final String sourceEtag, final AbfsRestOperation op, final String destination, - TracingContext tracingContext) { + TracingContext tracingContext, + final boolean isDir) { Preconditions.checkArgument(op.hasResult(), "Operations has null HTTP response"); - if ((op.isARetriedRequest()) - && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) - && isNotEmpty(sourceEtag)) { - + if (!(op.isARetriedRequest()) Review Comment: add a comment here. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ########## @@ -613,39 +644,58 @@ private void incrementAbfsRenamePath() { * Exceptions raised in the probe of the destination are swallowed, * so that they do not interfere with the original rename failures. * @param source source path + * @param sourceEtag etag of source file. may be null or empty * @param op Rename request REST operation response with non-null HTTP response * @param destination rename destination path - * @param sourceEtag etag of source file. may be null or empty * @param tracingContext Tracks identifiers for request header + * @param isDir is the source a file or directory * @return true if the file was successfully copied */ public boolean renameIdempotencyCheckOp( final String source, final String sourceEtag, final AbfsRestOperation op, final String destination, - TracingContext tracingContext) { + TracingContext tracingContext, + final boolean isDir) { Preconditions.checkArgument(op.hasResult(), "Operations has null HTTP response"); - if ((op.isARetriedRequest()) - && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) - && isNotEmpty(sourceEtag)) { - + if (!(op.isARetriedRequest()) + && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND)) { + // not an error + return false; + } + LOG.debug("Source not found on retry of rename({}, {}) isDir {} etag {}", + source, destination, isDir, sourceEtag); + if (isDir) { + // directory recovery is not supported. + // log and fail. + LOG.info("rename directory {} to {} failed; unable to recover", + source, destination); + return false; + } + if (isNotEmpty(sourceEtag)) { // Server has returned HTTP 404, which means rename source no longer // exists. Check on destination status and if its etag matches // that of the source, consider it to be a success. - LOG.debug("rename {} to {} failed, checking etag of destination", + // the source tag was either passed in from a manifest commit or + // retrieved when rename recovery is enabled. + LOG.info("rename {} to {} failed, checking etag of destination", source, destination); try { final AbfsRestOperation destStatusOp = getPathStatus(destination, false, tracingContext); final AbfsHttpOperation result = destStatusOp.getResult(); - return result.getStatusCode() == HttpURLConnection.HTTP_OK + final boolean recovered = result.getStatusCode() == HttpURLConnection.HTTP_OK && sourceEtag.equals(extractEtagHeader(result)); - } catch (AzureBlobFileSystemException ignored) { + LOG.info("File rename has taken place: recovery completed"); Review Comment: this log statement is incorrect. It will depend on the value of the recovered flag. > [ABFS]: RenameFilePath Source File Not Found (404) error in retry loop > ---------------------------------------------------------------------- > > Key: HADOOP-18425 > URL: https://issues.apache.org/jira/browse/HADOOP-18425 > Project: Hadoop Common > Issue Type: Bug > Components: fs/azure > Reporter: Sree Bhattacharyya > Assignee: Sree Bhattacharyya > Priority: Minor > Labels: pull-request-available > > RenameFilePath on its first try receives a Request timed out error with code > 500. On retrying the same operation, a Source file not found (404) error is > received. > Possible mitigation: Check whether etags remain the same before and after the > retry and accordingly send an Operation Successful result, instead of source > file not found. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org