anujmodi2021 commented on code in PR #7353:
URL: https://github.com/apache/hadoop/pull/7353#discussion_r1943022172
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/ConcurrentWriteOperationDetectedException.java:
##########
@@ -26,7 +26,10 @@
public class ConcurrentWriteOperationDetectedException
extends AzureBlobFileSystemException {
- public ConcurrentWriteOperationDetectedException(String message) {
- super(message);
+ private static final String ERROR_MESSAGE = "Parallel access to the create
path detected. Failing request "
+ + "to honor single writer semantics";
+
+ public ConcurrentWriteOperationDetectedException() {
Review Comment:
Let's add javadoc for the usage of this exception
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -555,58 +545,254 @@ public AbfsRestOperation createPath(final String path,
final AbfsRestOperation op = getAbfsRestOperation(
AbfsRestOperationType.PutBlob,
HTTP_METHOD_PUT, url, requestHeaders);
+ op.execute(tracingContext);
+ return op;
+ }
+
+ /**
+ * Checks if the specified path is a directory by listing its contents.
+ *
+ * @param path the path to check.
+ * @param tracingContext the tracing context for the service call.
+ * @return true if the path is a directory and contains entries, false
otherwise.
+ * @throws AzureBlobFileSystemException if the rest operation fails.
+ */
+ private boolean checkDirectoryByList(String path,
+ TracingContext tracingContext)
+ throws AzureBlobFileSystemException {
+ AbfsRestOperation listPathOp = listPath(path, false, 1, null,
+ tracingContext, false);
+ AbfsHttpOperation listPathResult = listPathOp.getResult();
+ if (listPathResult != null) {
+ // Determine if the path is a directory by checking if the list result
schema has any paths
+ return !listPathResult.getListResultSchema().paths().isEmpty();
+ }
+ return false;
+ }
+
+ /**
+ * Checks if the specified path exists as a directory.
+ *
+ * @param path the path of the directory to check.
+ * @param tracingContext the tracing context for the service call.
+ * @return true if the directory exists, false otherwise.
+ * @throws AzureBlobFileSystemException if the rest operation fails.
+ */
+ private boolean checkForDirectoryExistence(String path,
Review Comment:
Nit: I would also suggest to move all the private utility methods towards
end of file after alll public overrides
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -486,49 +506,19 @@ public AbfsRestOperation createPath(final String path,
* @param isAppendBlob whether the path is an append blob.
* @param eTag the eTag of the path.
* @param contextEncryptionAdapter the context encryption adapter.
- * @param tracingContext the tracing context.
- * @param isCreateCalledFromMarkers whether the create is called from
markers.
+ * @param tracingContext the tracing context for the service call.
* @return the executed rest operation containing the response from the
server.
* @throws AzureBlobFileSystemException if the rest operation fails.
*/
- public AbfsRestOperation createPath(final String path,
+ public AbfsRestOperation createPathRestOp(final String path,
final boolean isFile,
final boolean overwrite,
final AzureBlobFileSystemStore.Permissions permissions,
Review Comment:
permissions is not used. We can remove as its not a overriden method now.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -555,58 +545,254 @@ public AbfsRestOperation createPath(final String path,
final AbfsRestOperation op = getAbfsRestOperation(
AbfsRestOperationType.PutBlob,
HTTP_METHOD_PUT, url, requestHeaders);
+ op.execute(tracingContext);
+ return op;
+ }
+
+ /**
+ * Checks if the specified path is a directory by listing its contents.
+ *
+ * @param path the path to check.
+ * @param tracingContext the tracing context for the service call.
+ * @return true if the path is a directory and contains entries, false
otherwise.
+ * @throws AzureBlobFileSystemException if the rest operation fails.
+ */
+ private boolean checkDirectoryByList(String path,
+ TracingContext tracingContext)
+ throws AzureBlobFileSystemException {
+ AbfsRestOperation listPathOp = listPath(path, false, 1, null,
+ tracingContext, false);
+ AbfsHttpOperation listPathResult = listPathOp.getResult();
+ if (listPathResult != null) {
+ // Determine if the path is a directory by checking if the list result
schema has any paths
+ return !listPathResult.getListResultSchema().paths().isEmpty();
Review Comment:
Use `isNonEmptyListing()` here. Sometimes the list calls return no paths
with a Continuation Token. Those cases will be misssed.
It will alos make code cleaner as it will call list with result parsing
internally
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -555,58 +545,254 @@ public AbfsRestOperation createPath(final String path,
final AbfsRestOperation op = getAbfsRestOperation(
AbfsRestOperationType.PutBlob,
HTTP_METHOD_PUT, url, requestHeaders);
+ op.execute(tracingContext);
+ return op;
+ }
+
+ /**
+ * Checks if the specified path is a directory by listing its contents.
+ *
+ * @param path the path to check.
+ * @param tracingContext the tracing context for the service call.
+ * @return true if the path is a directory and contains entries, false
otherwise.
+ * @throws AzureBlobFileSystemException if the rest operation fails.
+ */
+ private boolean checkDirectoryByList(String path,
+ TracingContext tracingContext)
+ throws AzureBlobFileSystemException {
+ AbfsRestOperation listPathOp = listPath(path, false, 1, null,
+ tracingContext, false);
+ AbfsHttpOperation listPathResult = listPathOp.getResult();
+ if (listPathResult != null) {
+ // Determine if the path is a directory by checking if the list result
schema has any paths
+ return !listPathResult.getListResultSchema().paths().isEmpty();
+ }
+ return false;
+ }
+
+ /**
+ * Checks if the specified path exists as a directory.
+ *
+ * @param path the path of the directory to check.
+ * @param tracingContext the tracing context for the service call.
+ * @return true if the directory exists, false otherwise.
+ * @throws AzureBlobFileSystemException if the rest operation fails.
+ */
+ private boolean checkForDirectoryExistence(String path,
+ TracingContext tracingContext)
+ throws AzureBlobFileSystemException {
+ // Check if the directory contains any entries by listing its contents.
+ if (checkDirectoryByList(path, tracingContext)) {
Review Comment:
Seems like it can be simplified to
`return checkDirectoryByList() || checkEmptyDirectoryPathExists()`
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobRenameHandler.java:
##########
@@ -127,7 +127,7 @@ public boolean execute() throws
AzureBlobFileSystemException {
RenameAtomicity renameAtomicity = null;
if (pathInformation.getIsDirectory()
&& pathInformation.getIsImplicit()) {
- AbfsRestOperation createMarkerOp = getAbfsClient().createPath(
+ AbfsRestOperation createMarkerOp = getAbfsClient().createPathRestOp(
Review Comment:
Same here, why not call createDirectory here directly?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -555,58 +545,254 @@ public AbfsRestOperation createPath(final String path,
final AbfsRestOperation op = getAbfsRestOperation(
AbfsRestOperationType.PutBlob,
HTTP_METHOD_PUT, url, requestHeaders);
+ op.execute(tracingContext);
+ return op;
+ }
+
+ /**
+ * Checks if the specified path is a directory by listing its contents.
+ *
+ * @param path the path to check.
+ * @param tracingContext the tracing context for the service call.
+ * @return true if the path is a directory and contains entries, false
otherwise.
+ * @throws AzureBlobFileSystemException if the rest operation fails.
+ */
+ private boolean checkDirectoryByList(String path,
Review Comment:
nit: rename function to `checkIsDirectoryPath()` it should be sufficient to
convey the purpose of method I think
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobDeleteHandler.java:
##########
@@ -151,7 +151,7 @@ private void ensurePathParentExist()
throws AzureBlobFileSystemException {
if (!path.isRoot() && !path.getParent().isRoot()) {
try {
- getAbfsClient().createPath(path.getParent().toUri().getPath(),
+ getAbfsClient().createPathRestOp(path.getParent().toUri().getPath(),
Review Comment:
Why are we not calling `createDirectory` here?
--
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]
