-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-19658. ABFS:Create and rename idempotency for FNS Blob #7914
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
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
AbfsRestOperation statusOp = null; | ||
try { | ||
// Check if the file already exists by calling GetPathStatus | ||
statusOp = getPathStatus(path, false, tracingContext, 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.
In case of override true, flow might come here with already a Head call done on path.
Can we avoid this head call in that case?
assumeHnsDisabled(); | ||
// Create a spy of AzureBlobFileSystemStore | ||
AzureBlobFileSystemStore store = Mockito.spy(fs.getAbfsStore()); | ||
assumeBlobServiceType(); |
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.
We can move all assume before any other statement
// Create a spy of AzureBlobFileSystem | ||
try (AzureBlobFileSystem fs = Mockito.spy( | ||
(AzureBlobFileSystem) FileSystem.newInstance(getRawConfiguration()))) { | ||
assumeHnsDisabled(); |
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.
Same here, move all assume to first few lines
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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
LGTM
Some nits added
@@ -427,6 +427,8 @@ public static String containerProperty(String property, String fsName, String ac | |||
public static final String FS_AZURE_BLOB_DIR_DELETE_MAX_THREAD = "fs.azure.blob.dir.delete.max.thread"; | |||
/**Flag to enable/disable sending client transactional ID during create/rename operations: {@value}*/ | |||
public static final String FS_AZURE_ENABLE_CLIENT_TRANSACTION_ID = "fs.azure.enable.client.transaction.id"; | |||
/**Flag to enable/disable create idempotency during create operation: {@value}*/ | |||
public static final String FS_AZURE_ENABLE_CREATE_IDEMPOTENCY = "fs.azure.enable.create.idempotency"; |
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 is only for Blob Idempotency, may be we can keep config name accordingly
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
@@ -239,5 +239,7 @@ public final class FileSystemConfigurations { | |||
|
|||
public static final boolean DEFAULT_FS_AZURE_ENABLE_CLIENT_TRANSACTION_ID = true; | |||
|
|||
public static final boolean DEFAULT_FS_AZURE_ENABLE_ENABLE_CREATE_IDEMPOTENCY = 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.
Typo, ENABLE added twice
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
@@ -54,7 +56,15 @@ public static void dumpHeadersToDebugLog(final String origin, | |||
if (key == null) { | |||
key = "HTTP Response"; | |||
} | |||
String values = StringUtils.join(";", entry.getValue()); | |||
List<String> valuesList = entry.getValue(); |
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.
Why this change?
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.
Due to null pointer exceptions on enabling AbfsIoUtils logging if value is null.
🎊 +1 overall
This message was automatically generated. |
@@ -1043,6 +1047,12 @@ public String getAzureAtomicRenameDirs() { | |||
} | |||
|
|||
public boolean isConditionalCreateOverwriteEnabled() { | |||
// If either the configured FS service type or the ingress service type is BLOB, | |||
// conditional create-overwrite is not used. | |||
if (getFsConfiguredServiceType() == AbfsServiceType.BLOB |
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.
why is this change needed?
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 LGTM
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
============================================================
|
…#7914) Contributed by Anmol Asrani
This change introduces idempotency for create operation in the ABFS driver when using FNS-Blob. The update strengthens the client’s retry semantics to ensure that transient backend failures (e.g., HTTP 500) or ambiguous outcomes (where the client receives an error, but the operation has already succeeded on the service) are handled safely. This PR also includes test cases to validate the idempotency of rename operations.