-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Producer condition change + Rename Azcopy tests #127
Conversation
:::: AGGREGATED TEST RESULT :::: ============================================================
|
cf24cc3
to
0158163
Compare
NonHNS-SharedKey[ERROR] testUpdateDeepDirectoryStructureToRemote(org.apache.hadoop.fs.azurebfs.contract.ITestAbfsFileSystemContractDistCp) Time elapsed: 3.929 s <<< FAILURE! [WARNING] Tests run: 148, Failures: 0, Errors: 0, Skipped: 11 Time taken: 6 mins 58 secs.
|
@@ -519,12 +527,13 @@ private PathInformation getPathInformation(Path path, | |||
|
|||
return new PathInformation(true, | |||
abfsClient.checkIsDir(op.getResult()), | |||
extractEtagHeader(op.getResult())); | |||
extractEtagHeader(op.getResult()), | |||
op.getResult() instanceof AbfsHttpOperation.AbfsHttpOperationWithFixedResultForGetFileStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didnt get the need for instance check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPathStatus on blobClient will give instance of AbfsHttpOperationWithFixedResultForGetFileStatus for an implicit path. Hence used.
with blob endpoing :::: AGGREGATED TEST RESULT :::: ============================================================
|
:::: AGGREGATED TEST RESULT :::: ============================================================
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some thoughts and comments
@@ -417,6 +423,10 @@ public class AbfsConfiguration{ | |||
FS_AZURE_BLOB_DIR_DELETE_MAX_THREAD, DefaultValue = DEFAULT_FS_AZURE_BLOB_DELETE_THREAD) | |||
private int blobDeleteDirConsumptionParallelism; | |||
|
|||
@BooleanConfigurationValidatorAnnotation(ConfigurationKey = | |||
FS_AZURE_LEASE_CREATE_NON_RECURSIVE, DefaultValue = DEFAULT_FS_AZURE_LEASE_CREATE_NON_RECURSIVE) | |||
private boolean leaseOnCreateNonRecursive; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: boolean variable. Better to change to isLeaseOnCreateNonRecursiveEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken.
final TracingContext tracingContext, final boolean isNamespaceEnabled) | ||
throws AzureBlobFileSystemException { | ||
return createPath(path, isFile, overwrite, permissions, isAppendBlob, eTag, | ||
contextEncryptionAdapter, tracingContext, isNamespaceEnabled, false); | ||
AbfsLease abfsLease = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something...
Can you help me recall why we need this lease business now and not earlier??
Or was it just missed earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet it was missed. In case of createNonRecursive on atomic path, we have to take lease on the parent directory.
@@ -603,7 +603,7 @@ public void deleteFilesystem(TracingContext tracingContext) | |||
public OutputStream createFile(final Path path, | |||
final FileSystem.Statistics statistics, final boolean overwrite, | |||
final FsPermission permission, final FsPermission umask, | |||
TracingContext tracingContext) throws IOException { | |||
final boolean isRecursiveCreate, TracingContext tracingContext) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter passed to store has name isNonRecursiveCreate
and parameter accepted in store has name isRecursiveCreate
.
This seems confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems buggy as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with the confusion. It has to be all about nonRecursiveCreate. Can you please explain what seems wrong here please :).
@@ -417,7 +426,7 @@ public FSDataOutputStream createNonRecursive(final Path f, final FsPermission pe | |||
+ f.getName() + " because parent folder does not exist."); | |||
} | |||
|
|||
return create(f, permission, overwrite, bufferSize, replication, blockSize, progress); | |||
return createInternal(f, permission, overwrite, blockSize, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like these parameters were not used. Should we still keep them to reduce unnecessary diffs??
No, issues in removing as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken.
return createInternal(f, permission, overwrite, blockSize, false); | ||
} | ||
|
||
private FSDataOutputStream createInternal(final Path f, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this refraction we are doing only for Blob Endpoint. this new parameter is not used by DFS Client. Can we have these handling only in ABFSBlobClient?
Let's discuss this once offline if its possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new field is just to tell BlobClient, if the createPath is for createNonRecursive HDFS API or create HDFS API. The required orchestration for blob on createNonRecursive is done in blobClient only. This field is just to propagate the information to the client about what HDFS API has invoked it.
…ave lease before checking parent existence so that it can be ensured that no parallel rename is taking place on the atomic dir. Added tests; removed unrequried code
blob non-hns::::: AGGREGATED TEST RESULT :::: ============================================================
|
on dfs endpoint: :::: AGGREGATED TEST RESULT :::: ============================================================
|
@@ -321,6 +320,7 @@ public AbfsRestOperation listPath(final String relativePath, | |||
/** | |||
* Get Rest Operation for API <a href = https://learn.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/path/create></a>. | |||
* Create a path (file or directory) in the current filesystem. | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes in this file, additional changes can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taken.
@@ -451,6 +456,37 @@ public abstract AbfsRestOperation createPath(final String path, | |||
final ContextEncryptionAdapter contextEncryptionAdapter, | |||
final TracingContext tracingContext, boolean isNamespaceEnabled) throws AzureBlobFileSystemException; | |||
|
|||
public AbfsRestOperation createNonRecursivePath(final String pathStr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add javadocs for this method
umask), | ||
false, | ||
null, null, tracingContext, isNamespaceEnabled); | ||
return createAbfsOutputStreamInstance(statistics, tracingContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is no check for appendBlob and contextEncryptionAdapter needed for non recursive create ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected.
getPathStatus(parentPath.toString(), false, tracingContext, | ||
contextEncryptionAdapter); | ||
} catch (AbfsRestOperationException ex) { | ||
if (ex.getStatusCode() == HttpURLConnection.HTTP_OK) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is not clear if it is 200 response why throw exception ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be 404. corrected.
} | ||
throw ex; | ||
} finally { | ||
abfsCounters.incrementCounter(CALL_GET_FILE_STATUS, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why getFileStatus increment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetFileStatus API is getting called, and there is a test that asserts it. Hence added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see only a GetPathStatus API call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, its for that, in trunk, it calls tryGetFileStatus -> getFileStatus -> store.getFileStatus -> client.getPathStatus
isAppendBlob, eTag, contextEncryptionAdapter, tracingContext, | ||
isNamespaceEnabled); | ||
} finally { | ||
abfsCounters.incrementCounter(CALL_CREATE, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This finally method should also release lease
} | ||
throw ex; | ||
} finally { | ||
abfsCounters.incrementCounter(CALL_GET_FILE_STATUS, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also dont see any call for this API
@anmolanmol1234 @anujmodi2021 , thanks for all the reviews! Have done the following for fs.createNonRecursive: instead of
now doing:
Here CreateNonRecursiveCheckActionTaker is a closable object. Now, store.createNonRecursivePreCheck will forward to client.createNonRecursivePreCheck for endpoint related check. The precheck can attain some resources like lease (depends on conditions and endpoint) until create is done. CreateNonRecursiveCheckActionTaker will have info of that resource. On close of CreateNonRecursiveCheckActionTaker , it will release that resource (here lease). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks for taking comments and resolving my queries.
Changes:
dfs full run:
:::: AGGREGATED TEST RESULT ::::
============================================================
HNS-OAuth
[WARNING] Tests run: 147, Failures: 0, Errors: 0, Skipped: 4
[WARNING] Tests run: 740, Failures: 0, Errors: 0, Skipped: 153
[WARNING] Tests run: 414, Failures: 0, Errors: 0, Skipped: 48
============================================================
HNS-SharedKey
[ERROR] testUpdateDeepDirectoryStructureToRemote(org.apache.hadoop.fs.azurebfs.contract.ITestAbfsFileSystemContractDistCp) Time elapsed: 3.967 s <<< FAILURE!
[ERROR] testHttpReadTimeout(org.apache.hadoop.fs.azurebfs.ITestAzureBlobFileSystemE2E) Time elapsed: 6.34 s <<< ERROR!
[WARNING] Tests run: 147, Failures: 0, Errors: 0, Skipped: 5
[ERROR] Tests run: 740, Failures: 0, Errors: 1, Skipped: 105
[ERROR] Tests run: 414, Failures: 1, Errors: 0, Skipped: 35
============================================================
NonHNS-SharedKey
[ERROR] testHttpReadTimeout(org.apache.hadoop.fs.azurebfs.ITestAzureBlobFileSystemE2E) Time elapsed: 6.691 s <<< ERROR!
[WARNING] Tests run: 147, Failures: 0, Errors: 0, Skipped: 10
[ERROR] Tests run: 724, Failures: 0, Errors: 1, Skipped: 346
[WARNING] Tests run: 414, Failures: 0, Errors: 0, Skipped: 38
============================================================
AppendBlob-HNS-OAuth
[WARNING] Tests run: 147, Failures: 0, Errors: 0, Skipped: 4
[WARNING] Tests run: 740, Failures: 0, Errors: 0, Skipped: 157
[WARNING] Tests run: 414, Failures: 0, Errors: 0, Skipped: 72
Time taken: 26 mins 43 secs.
blob test run:
============================================================
NonHNS-SharedKey
[ERROR] testValidateSeekBounds(org.apache.hadoop.fs.azurebfs.ITestAzureBlobFileSystemRandomRead) Time elapsed: 2.515 s <<< FAILURE!
[WARNING] Tests run: 147, Failures: 0, Errors: 0, Skipped: 11
[ERROR] Tests run: 724, Failures: 1, Errors: 0, Skipped: 278
[WARNING] Tests run: 414, Failures: 0, Errors: 0, Skipped: 39
Time taken: 7 mins 23 secs.
azureuser@pranav-ind-vm:~/AbfsHadoop/hadoop-tools/hadoop-azure$ git log
commit 00cec1c (HEAD -> sp/azcopyTests, origin/sp/azcopyTests)
Author: Pranav Saxena <>
Date: Mon Jul 8 06:51:41 2024 -0700