HADOOP-17590 ABFS: Introduce Lease Operations with Append to provide single writer semantics #3026
Conversation
|
a. HNS account + OAuth config Appendblob-HNS-OAuthResults: Tests run: 97, Failures: 0, Errors: 0, Skipped: 0 Failures: Tests run: 565, Failures: 1, Errors: 0, Skipped: 98 Errors: Tests run: 261, Failures: 0, Errors: 2, Skipped: 74 HNS-OAuthResults: Tests run: 97, Failures: 0, Errors: 0, Skipped: 0 Tests run: 565, Failures: 0, Errors: 0, Skipped: 98 Errors: Tests run: 261, Failures: 0, Errors: 2, Skipped: 50 HNS-SharedKeyResults: Tests run: 97, Failures: 0, Errors: 0, Skipped: 0 Errors: Tests run: 565, Failures: 0, Errors: 1, Skipped: 67 Errors: Tests run: 261, Failures: 0, Errors: 3, Skipped: 40 NonHNS-SharedKeyResults: Tests run: 97, Failures: 0, Errors: 0, Skipped: 0 Errors: Tests run: 565, Failures: 0, Errors: 5, Skipped: 285 Errors: Tests run: 261, Failures: 0, Errors: 3, Skipped: 40 |
snehavarma
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| return lease; | ||
| } | ||
|
|
||
| private AbfsLease maybeCreateLease(String relativePath, boolean isNamespaceEnabled) |
There was a problem hiding this comment.
Merge maybeCreateLease and maybeCreateInfiniteLease
| } | ||
| } | ||
|
|
||
| if (lease != null && lease.getLeaseID() != null && !lease.getLeaseID().isEmpty()) { |
There was a problem hiding this comment.
redundant code in flush and append. Create a function
| triggerConditionalCreateOverwrite = true; | ||
| } | ||
|
|
||
| AbfsLease lease = maybeCreateFiniteLease(relativePath, isNamespaceEnabled); |
There was a problem hiding this comment.
If the path qualifies for infinteLease as per config settings, this will still create a finite lease ?
Call to a method createLease() that will first check for infiniteLease setting before defaulting to finite lease (if write lease config is enabled), would be the expectation ?
There was a problem hiding this comment.
No it will not, it returns a null object if path qualifies for infinite lease
|
|
||
| AbfsLease lease = maybeCreateLease(relativePath); | ||
|
|
||
| if (lease == null) { |
There was a problem hiding this comment.
Acquiring lease along with create/append enables atomic fetch of lease with the store operation and prevent any parallel writers. If configured for infinite lease, the path creation will happen without bundled lease acquiry and rely on later acquireLease API call in AbfsLease. Infinite lease flow should purely be on acquireLease API only if fs.azure.write.enforceLease is off.
| return null; | ||
| AbfsLease lease = null; | ||
| if (!enableInfiniteLease && abfsConfiguration.isLeaseEnforced() && isNamespaceEnabled) { | ||
| lease = new AbfsLease(client, relativePath, false); |
There was a problem hiding this comment.
Are there scenarios where a finite lease created needs to explicitly call on acquireLease API ? If that dependency isnt there, it would be better to create 2 child classes to AbfsLease as AbfsInfiniteLeaseV1 and AbfsApiBundledLease.
|
|
||
| public AbfsRestOperation append(final String path, final byte[] buffer, | ||
| AppendRequestParameters reqParams, final String cachedSasToken) | ||
| AppendRequestParameters reqParams, final String cachedSasToken, AbfsLease lease) |
There was a problem hiding this comment.
Lease should be a member of AppendRequestParameters.
| } | ||
| } | ||
|
|
||
| if (lease != null && lease.getLeaseID() != null && !lease.getLeaseID().isEmpty()) { |
There was a problem hiding this comment.
Add below member functions to AbfsLease :
- boolean hasValidLease() - which will return value of ( lease.getLeaseID() != null && !lease.getLeaseID().isEmpty())
- AddLeaseHeaders(List requestHeaders) - and move the requestHeader conditional header add logic into it
| if (leaseId != null) { | ||
| requestHeaders.add(new AbfsHttpHeader(X_MS_LEASE_ID, leaseId)); | ||
|
|
||
| if (lease != null && lease.getLeaseID() != null && !lease.getLeaseID().isEmpty()) { |
| } | ||
| } | ||
|
|
||
| private void updateRequestHeaders() { |
There was a problem hiding this comment.
AbfsRestOperation is not supposed to have any API specific implementations or handling. Come up with a LeaseRetryPolicy that will be used when executing operations that pass in leaseId. That can control retry count to 2 or how many ever reduced number of retries needed. Post that, if the request still fails, retry policy should exit AbfsRestOperation and abfsClient should take care of retrying from the respective method post updating request headers.
snvijaya
left a comment
snvijaya
left a comment
There was a problem hiding this comment.
Please check the comments.
| } | ||
|
|
||
| AbfsLease lease = maybeCreateLease(relativePath); | ||
| AbfsLease lease = maybeCreateLease(relativePath, getIsNamespaceEnabled()); |
There was a problem hiding this comment.
make it only for hns enabled
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
3 participants
The lease operations have been introduced as part of Create, Append, Flush to ensure the single writer semantics.
Testing Done:
Region: Canary EastUs2euap
a. HNS account + OAuth config
b. HNS account + Shared Key config
c. Non-HNS account + SharedKey config
d. AppendBlob+HNS+Oauth config
Failures seen - testReadAndWriteWithDifferentBufferSizesAndSeek, ITestAbfsFileSystemContractDistCp, ITestAbfsFileSystemContractSecureDistCp, TestAbfsStreamOps with appendblob, testBlobBackCompatibility, testRandomRead & WasbAbfsCompatibility with non-HNS account