HADOOP-18425. ABFS rename recovery #5494
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If "fs.azure.enable.rename.resilience" is true, then do a HEAD of the source file before the rename, which can then be used to recover from the failure, as the manifest committer does (HADOOP-18163). Change-Id: Ia417f1501f7274662eb9ff919c6378fb913b476b HADOOP-18425. ABFS rename resilience through etags only get the etag on HNS stores Change-Id: I9faffa78294e1782f0b2db3d1c997ec3fe53637c
1. move config checks of rename resilience flag into AbfsClient 2. only getPathStatus on rename if enabled 3. resilience handling logs when unable to recover from a dir 4. and when it successfully recovers a file. Change-Id: I58b5f11e4c9b7c1a1d809d2db47a3cafe51f2060
Contributor
Author
|
tests good except for timeout in all the lease runs not sure what is up there. I had been playing with leases recently but my site settings haven't set any. the timeouts are only 30s, so maybe its just a slow test run. no vpn invoved though |
Contributor
Author
|
and yes, a test run on its own works |
Contributor
Author
|
reviews encouraged from people, especially @sreeb-msft @snvijaya @saxenapranav @hiteshs @mukund-thakur @mehakmeet @taklwu |
|
💔 -1 overall
This message was automatically generated. |
Contributor
mukund-thakur
left a comment
mukund-thakur
left a comment
There was a problem hiding this comment.
added some minor comments.
| this.statistics = fs.statistics; | ||
| } | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
accidental edit; will revert
| final boolean recovered = result.getStatusCode() == HttpURLConnection.HTTP_OK | ||
| && sourceEtag.equals(extractEtagHeader(result)); | ||
| } catch (AzureBlobFileSystemException ignored) { | ||
| LOG.info("File rename has taken place: recovery completed"); |
Contributor
There was a problem hiding this comment.
this log statement is incorrect. It will depend on the value of the recovered flag.
Comment on lines
678
to
680
| // 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. |
Contributor
There was a problem hiding this comment.
these comments not valid here.
Contributor
Author
There was a problem hiding this comment.
will cut back
| && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) | ||
| && isNotEmpty(sourceEtag)) { | ||
|
|
||
| if (!(op.isARetriedRequest()) |
snvijaya
reviewed
Mar 20, 2023
| // etag passed in, so source is a file | ||
| final boolean hasEtag = isEmpty(sourceEtag); | ||
| boolean isDir = !hasEtag; | ||
| if (!hasEtag && renameResilience) { |
Contributor
There was a problem hiding this comment.
sourceEtag should be passed down only for HNS account as eTag does not remain the same when its FNS (as rename = copy to destination and delete source).
Contributor
Author
There was a problem hiding this comment.
yeah, but AFAIK there's no way to check in the client whether it is available.
it can only get passed in through the manifest committer, and as ABFS.createResilientCommitSupport() requires etag preservation, it won't be doing it on a non-HNS store.
saxenapranav
suggested changes
Mar 20, 2023
| /** | ||
| * Enable resilient rename. | ||
| */ | ||
| private boolean renameResilience; |
Contributor
There was a problem hiding this comment.
nit: lets have it final.
Comment on lines
49
to
51
| import org.apache.hadoop.fs.EtagSource; | ||
| import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation; | ||
| import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation; |
Contributor
There was a problem hiding this comment.
imports not required.
| // not an error | ||
| return false; | ||
| } | ||
| LOG.debug("Source not found on retry of rename({}, {}) isDir {} etag {}", |
Contributor
There was a problem hiding this comment.
this can come even if op was retried and statusCode is not equal to 404
Contributor
Author
There was a problem hiding this comment.
the existing code only handles 404. Are you saying there are other errors we should look for, such as some 500+ value? if so: which ones
Contributor
There was a problem hiding this comment.
this function gets invoked for any AbfsBlobFileSystemException whether it be because of 404 or any other: https://github.com/steveloughran/hadoop/blob/70a8d0fb3956b2a0e2c343373475ef7a0e75ab08/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java#L607.
now because at line 668, it is because :
op.isARetriedRequest() || op.getResult().getStatusCode() != HttpUrlConnection.HTTP_NOT_FOUND
any retried operation will come on this line.
Comment on lines
668
to
676
| 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; | ||
| } |
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment.
ok, let me copy and paste again. i think i was trying to do the inversion logic but may have stopped partway through.
| source, destination); | ||
| return false; | ||
| } | ||
| if (isNotEmpty(sourceEtag)) { |
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment.
yes, but I want to log slightly differently. though actually it is confusion as isDir is true if resilience is off. will cut
| // specifying AbfsHttpOperation mock behavior | ||
|
|
||
| // mock object representing the 404 path not found result | ||
| AbfsHttpOperation mockHttp404Op = Mockito.mock(AbfsHttpOperation.class); |
Contributor
There was a problem hiding this comment.
It would be awesome if we can use server integration in picture. Have added a comment on @sreeb-msft 's pr #5488 (comment). Suggested changes in saxenapranav@5247e12
| final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders(); | ||
|
|
||
| // etag passed in, so source is a file | ||
| final boolean hasEtag = isEmpty(sourceEtag); |
Contributor
There was a problem hiding this comment.
should it be
hasEtag = !isEmpty(sourceEtag)
Contributor
Author
There was a problem hiding this comment.
ooh, nice catch. scarily nice. what happens when i code on a weekend
+ new itest, ITestAbfsContractRenameWithoutResilience this disables resilience and so verifies the normal codepath is still good. Change-Id: Ib2663c70afb112c9430043e94d75e9ddf7b3c313
Change-Id: I1db3878ee12ea082e00438781e1ae86af9850ff7
Integration testing all happy; had to do some work to get my auth mechanism work through the tests. Added test for dir handling, and commit renaming working through the failure. First time it's had this test, fwiw Change-Id: I89f7763d03d1a24a1a43361b001bfa5830804bc1
Contributor
Author
|
running the itests with the new pr and it is interesting because: when resilience is enabled, ITestAbfsFileSystemContractRename fail because of different outcomes from what is defined in the contract XML file. that is actually what is expected according to https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/filesystem/filesystem.html#boolean_rename.28Path_src.2C_Path_d.29, so I'm not sure why it doesn't happen today. will investigate |
Contributor
Author
|
so hdfs returns false if source not found, or dest exists <property>
<name>fs.contract.rename-returns-false-if-dest-exists</name>
<value>true</value>
</property>
<property>
<name>fs.contract.rename-returns-false-if-source-missing</name>
<value>true</value>
</property>abfs does the same. s3a client blows up with meaningful errors <property>
<name>fs.contract.rename-returns-false-if-source-missing</name>
<value>false</value>
</property>
<property>
<name>fs.contract.rename-returns-false-if-dest-exists</name>
<value>false</value>
</property>I'm more in favour of the "blow up" strategy; if you look at almost all uses of rename it is code like: if (!rename(src, dest)) throw new Exception("rename failed we don't know why")pand nobody ever seems to complain about the s3a failure...but then of course rename is broken there for other reasons, so maybe code just avoids it (e.g. committers). |
Change-Id: Iceb0042b2d97725d0864d138d3a522f29fb5c867
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
mehakmeet
reviewed
Mar 21, 2023
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.apache.hadoop.classification.VisibleForTesting; | ||
| import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException; |
Contributor
There was a problem hiding this comment.
import seem to be in the wrong place here.
| HadoopExecutors.newScheduledThreadPool(this.abfsConfiguration.getNumLeaseThreads(), tf)); | ||
| // rename resilience | ||
| renameResilience = abfsConfiguration.getRenameResilience(); | ||
| LOG.debug("Rename resilience is {}",renameResilience); |
| } | ||
|
|
||
| /** | ||
| * Even with failures, having |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
This is @sreeb-msft's PR of #5488 with
The handling is now exclusively in the AbfsClient class.
How was this patch tested?
abfs test in progress; azure cardiff.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?