Hadoop-17215. ABFS: Disable default create overwrite #2246
Closed
Conversation
This file contains 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
snvijaya
commented
Aug 26, 2020
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
snvijaya
commented
Aug 26, 2020
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
snvijaya
commented
Aug 26, 2020
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
snvijaya
commented
Aug 26, 2020
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
snvijaya
force-pushed
the
HADOOP-17215
branch
2 times, most recently
from
ac6fd41
to
773f9bd
Compare
snvijaya
changed the title
|
Test results from accounts in East US 2. From HNS Enabled account with OAuth credential settings[INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 From HNS Enabled account with SharedKey credential settings[INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 From HNS Non-enabled account with OAuth credential settings[INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 From HNS Non-enabled account with SharedKey credential settings[INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 The fix for the failing test is tracked under: https://issues.apache.org/jira/browse/HADOOP-17229 |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
If the main purpose is to resolve the potential race condition, this fix seems not enough as the race condition can still happen in the retry process for the second "create with override" call in catch logic. And we introduce an extra call for create operation. |
DadanielZ
reviewed
Aug 31, 2020
| isFirstAttemptToCreateWithoutOverwrite = false; | ||
| // was a first attempt made to create without overwrite. Now try again | ||
| // with overwrite now. | ||
| op = createPathImpl(path, abfsUriQueryBuilder, true, permission, umask); |
There was a problem hiding this comment.
it is more likely the race condition can still happen in the retry process for this call.
There was a problem hiding this comment.
PR has been modified to include the step to fetch eTag to ensure that the create overwrite=true retry case is handled too.
shanemkm
reviewed
Sep 1, 2020
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
shanemkm
reviewed
Sep 1, 2020
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
snvijaya
force-pushed
the
HADOOP-17215
branch
from
137ca9c
to
6ff28f1
Compare
|
Non-HNS account with SharedKey [INFO] [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 246 [WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 24` Non-HNS account with OAuth [INFO] [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 250 WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 140` HNS account with SharedKey [INFO] [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 41 [WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 24` HNS account with OAuth [INFO] [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 74 [WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 140` |
|
🎊 +1 overall
This message was automatically generated. |
| public AbfsRestOperation createPath(final String path, final boolean isFile, final boolean overwrite, | ||
| final String permission, final String umask, | ||
| final boolean isAppendBlob) throws AzureBlobFileSystemException { | ||
| public AbfsRestOperation createPath(final String path, |
There was a problem hiding this comment.
There is reformatting of the previous code here that is unnecessary, but more importantly can cause merge conflicts when back porting and can introduce regressions if not carefully reviewed or caught by existing test automation. It is better to leave the old code as-is, but only update what must be updated. Just my thoughts.
There was a problem hiding this comment.
Only update to createPath in AbfsClient with the new iteration is the new eTag parameter now. Have tried to minimize code changes to existing code only where needed.
Comment on lines
314
to
327
| // Fetch eTag | ||
| try { | ||
| op = getPathStatus(path, false); | ||
| } catch (AbfsRestOperationException ex) { | ||
| if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { | ||
| // Is a parallel access case, as file which was found to be | ||
| // present went missing by this request. | ||
| throw new ConcurrentWriteOperationDetectedException( | ||
| "Parallel access to the create path detected. Failing request " | ||
| + "to honor single writer semantics"); | ||
| } else { | ||
| throw ex; | ||
| } | ||
| } |
There was a problem hiding this comment.
The original design was such that AbfsClient is a thin client over the REST API, and AzureBlobFileSystemStore is where the app logic lived, such as handling continuation tokens or making multiple calls such as getting the etag to use it in a conditional request. I think we should stick to the original design and move this fancy logic to AzureBlobFileSystemStore and expose an option on AbfsClient to take an optional condition (the etag) when creating a file. In this way, the update to the AbfsClient would be a single line to add the optional "If-Match: E-Tag" request header, but there would be a new method in AzureBlobFileSystemStore that implements the new conditional overwrite behavior.
There was a problem hiding this comment.
Updated PR with the recommendation.
| */ | ||
| @org.apache.hadoop.classification.InterfaceAudience.Public | ||
| @org.apache.hadoop.classification.InterfaceStability.Evolving | ||
| public class ConcurrentWriteOperationDetectedException |
There was a problem hiding this comment.
HDFS would simply acquire a lease and overwrite the file or fail to acquire the lease and throw an IOException, so what we're really pointing out here is the need for Azure Blob Storage to support lease atomically on file creation and for ABFS to use leases when writing to files so that it can uphold the single writer semantics. We knew this was needed from the beginning but the work has not been done.
There was a problem hiding this comment.
Seems we should be doing something like https://issues.apache.org/jira/browse/HADOOP-16948 instead.
There was a problem hiding this comment.
Will check on what the scope for lease support is on server and how it can be extended to support this PR scenario when we plan for adopting the new lease related changes in server.
|
Repeated the HNS tests: [INFO] [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 41 [WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 24 HNS account with OAuth [INFO] [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [WARNING] Tests run: 457, Failures: 0, Errors: 0, Skipped: 74 [WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 140 |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
commit e31a636 |
This was referenced Apr 14, 2022
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Filesystem Create APIs that do not accept an argument for overwrite flag end up defaulting it to true.
We are observing that request count of creates with overwrite=true is more and primarily because of the default setting of the flag is true of the called Create API. When a create with overwrite ends up timing out, we have observed that it could lead to race conditions between the first create and retried one running almost parallel.
To avoid this scenario for create with overwrite=true request, ABFS driver will always attempt to create without overwrite. If the create fails due to fileAlreadyPresent, it will fetch the ETag of the current file and then resend the request with overwrite=true with condition to overwrite only if ETag matches.
This change will be guarded with a config if for any reason this behaviour needs to be reverted.
Tests for file and directory creations with and without overwrite along with file pre-existing or not combinations have been tested.