[
https://issues.apache.org/jira/browse/HADOOP-18425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17702485#comment-17702485
]
ASF GitHub Bot commented on HADOOP-18425:
-----------------------------------------
saxenapranav commented on code in PR #5494:
URL: https://github.com/apache/hadoop/pull/5494#discussion_r1141630233
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -105,6 +106,11 @@ public class AbfsClient implements Closeable {
private final ListeningScheduledExecutorService executorService;
+ /**
+ * Enable resilient rename.
+ */
+ private boolean renameResilience;
Review Comment:
nit: lets have it final.
##########
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 {}",
Review Comment:
this can come even if op was retried and statusCode is not equal to 404
##########
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)) {
Review Comment:
As per code in renamePath(),
```
isDir = !isNotEmpty(sourceEtag)
```
Should we use the same relation in this method to reduce confusion, instead
of having a new argument `isDir`.
##########
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;
+ }
Review Comment:
this becomes equavalent to:
```
op.isARetriedRequest() || op.getResult().getStatusCode !=
HttpURLConnection.HTTP_NOT_FOUND
```
The older condition:
```
if ((op.isARetriedRequest())
&& (op.getResult().getStatusCode() ==
HttpURLConnection.HTTP_NOT_FOUND)
```
is changed.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsRenameRetryRecovery.java:
##########
@@ -123,6 +138,112 @@ public void testRenameFailuresDueToIncompleteMetadata()
throws Exception {
}
+ AbfsClient getMockAbfsClient() throws IOException {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ // specifying AbfsHttpOperation mock behavior
+
+ // mock object representing the 404 path not found result
+ AbfsHttpOperation mockHttp404Op = Mockito.mock(AbfsHttpOperation.class);
Review Comment:
It would be awesome if we can use server integration in picture. Have added
a comment on @sreeb-msft 's pr
https://github.com/apache/hadoop/pull/5488#discussion_r1141679607. Suggested
changes in
https://github.com/saxenapranav/hadoop/commit/5247e12bf2b96f8397d4c6df0865117a18dd13fd
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -46,6 +46,9 @@
import javax.annotation.Nullable;
import org.apache.hadoop.classification.VisibleForTesting;
+import org.apache.hadoop.fs.EtagSource;
+import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation;
+import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation;
Review Comment:
imports not required.
> [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: [email protected]
For additional commands, e-mail: [email protected]
