anujmodi2021 commented on code in PR #7364:
URL: https://github.com/apache/hadoop/pull/7364#discussion_r1968915858
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/AbfsDriverException.java:
##########
@@ -51,4 +51,12 @@ public AbfsDriverException(final Exception innerException,
final String activity
: ERROR_MESSAGE + ", rId: " + activityId,
null);
}
+
+ public AbfsDriverException(final String errorMessage, final Exception
innerException) {
Review Comment:
I was wondering if the exception will have reqId or not.
Req id is part of abfsHttpOperation. Can we also pass down activity Id and
append it to error message?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java:
##########
@@ -705,6 +711,30 @@ public AbfsClientRenameResult renamePath(
throw e;
}
+ // recovery using client transaction id only if it is a retried request.
+ if (op.isARetriedRequest() && clientTransactionId != null
+ && SOURCE_PATH_NOT_FOUND.getErrorCode().equalsIgnoreCase(
+ op.getResult().getStorageErrorCode())) {
+ try {
+ final AbfsHttpOperation abfsHttpOperation =
+ getPathStatus(destination, false,
+ tracingContext, null).getResult();
+ if (clientTransactionId.equals(
+ abfsHttpOperation.getResponseHeader(
+ X_MS_CLIENT_TRANSACTION_ID))) {
+ return new AbfsClientRenameResult(
+ getSuccessOp(AbfsRestOperationType.RenamePath,
+ HTTP_METHOD_PUT, url, requestHeaders), true,
+ isMetadataIncompleteState);
+ }
+ } catch (AzureBlobFileSystemException exception) {
+ throw new AbfsDriverException(
+ "Error in getPathStatus while recovering from rename failure.",
Review Comment:
Let's define error string as constant in `AbfsErrors` class and add a
mockito test to verify we get the proper exception thrown, if not already there.
Same for create as well.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java:
##########
@@ -415,7 +416,9 @@ public AbfsRestOperation createPath(final String path,
String existingResource =
op.getResult().getResponseHeader(X_MS_EXISTING_RESOURCE_TYPE);
if (existingResource != null && existingResource.equals(DIRECTORY)) {
- return op; //don't throw ex on mkdirs for existing directory
+ //don't throw ex on mkdirs for existing directory
+ return getSuccessOp(AbfsRestOperationType.CreatePath,
Review Comment:
This is good function to have a cleaner code, let's define it in base class
and use it everywhere we are setting hard result. Even in Blob Client
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemRename.java:
##########
@@ -1766,4 +1745,69 @@ public void getClientTransactionIdAfterRename() throws
Exception {
.isEqualTo(clientTransactionId[0]);
}
}
+
+ @Test
+ public void failureInGetPathStatusDuringRenameRecovery() throws Exception {
+ try (AzureBlobFileSystem fs = getFileSystem()) {
+ assumeRecoveryThroughClientTransactionID(false);
+ AbfsDfsClient abfsDfsClient = (AbfsDfsClient)
Mockito.spy(fs.getAbfsClient());
+ fs.getAbfsStore().setClient(abfsDfsClient);
+ final String[] clientTransactionId = new String[1];
+ mockAddClientTransactionIdToHeader(abfsDfsClient, clientTransactionId);
+ mockRetriedRequest(abfsDfsClient, new ArrayList<>());
+ boolean[] flag = new boolean[1];
+ Mockito.doAnswer(getPathStatus -> {
+ if (!flag[0]) {
+ flag[0] = true;
+ throw new AbfsRestOperationException(HTTP_CLIENT_TIMEOUT, "", "",
new Exception());
+ }
+ return getPathStatus.callRealMethod();
+ }).when(abfsDfsClient).getPathStatus(
+ Mockito.nullable(String.class), Mockito.nullable(Boolean.class),
+ Mockito.nullable(TracingContext.class),
+ Mockito.nullable(ContextEncryptionAdapter.class));
+
+ Path sourceDir = path("/testSrc");
+ assertMkdirs(fs, sourceDir);
+ String filename = "file1";
+ Path sourceFilePath = new Path(sourceDir, filename);
+ touch(sourceFilePath);
+ Path destFilePath = new Path(sourceDir, "file2");
+
+ String errorMessage = intercept(AbfsDriverException.class,
+ () -> fs.rename(sourceFilePath, destFilePath)).getErrorMessage();
+
+ Assertions.assertThat(errorMessage)
+ .describedAs("getPathStatus should fail while recovering")
+ .contains("Error in getPathStatus while recovering from rename
failure.");
Review Comment:
Same as above
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java:
##########
@@ -2213,6 +2193,49 @@ public void testClientTransactionIdAfterTwoCreateCalls()
throws Exception {
}
}
+ /**
+ * Test to verify that the client transaction ID is included in the response
header
+ * during the creation of a new file in Azure Blob Storage.
+ * <p>
+ * This test ensures that when a new file is created, the Azure Blob
FileSystem client
+ * correctly includes the client transaction ID in the response header for
the created file.
+ * The test uses a configuration where client transaction ID is enabled and
verifies
+ * its presence after the file creation operation.
+ * </p>
+ *
+ * @throws Exception if any error occurs during test execution
+ */
+ @Test
+ public void failureInGetPathStatusDuringCreateRecovery() throws Exception {
+ try (AzureBlobFileSystem fs = getFileSystem()) {
+ assumeRecoveryThroughClientTransactionID(true);
+ final String[] clientTransactionId = new String[1];
+ AbfsDfsClient abfsDfsClient = mockIngressClientHandler(fs);
+ mockAddClientTransactionIdToHeader(abfsDfsClient, clientTransactionId);
+ mockRetriedRequest(abfsDfsClient, new ArrayList<>());
+ boolean[] flag = new boolean[1];
+ Mockito.doAnswer(getPathStatus -> {
+ if (!flag[0]) {
+ flag[0] = true;
+ throw new AbfsRestOperationException(HTTP_CLIENT_TIMEOUT, "", "",
new Exception());
+ }
+ return getPathStatus.callRealMethod();
+ }).when(abfsDfsClient).getPathStatus(
+ Mockito.nullable(String.class), Mockito.nullable(Boolean.class),
+ Mockito.nullable(TracingContext.class),
+ Mockito.nullable(ContextEncryptionAdapter.class));
+
+ final Path nonOverwriteFile = new Path(
+ "/NonOverwriteTest_FileName_" + UUID.randomUUID());
+ String errorMessage = intercept(AbfsDriverException.class,
+ () -> fs.create(nonOverwriteFile, false)).getErrorMessage();
+
+ Assertions.assertThat(errorMessage)
+ .describedAs("getPathStatus should fail while recovering")
+ .contains("Error in getPathStatus while recovering from create
failure.");
Review Comment:
Okay seems like this is the test I was referring to. Let's use constants
here for error message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
