Rename Delete on Blob endpoint. #117

saxenapranav · 2024-06-13T05:28:23Z

Enable rename and delete over Blob endpoint. The endpoint doesnot support rename API and not directory-delete.

There shall be no change until client layer, wherein, in the AbfsBlobClient#rename(), we will create an instance of BlobRenameHandler, and in AbfsBlobClient#delete(), we will create an instance of BlobDeleteHandler. These two new classes would own the orchestration logic for doing rename and delete, also they would own the HDFS contract prechecks. DFS endpoint performs the contract checks but not blob endpoint, hence, the handler would have to do prechecks and the postchecks.

How to directory operations?
In both of them, there is a need of listing that needs to be done by the client and then take actions on them. But, there is a problem with iterative listing and then taking action:

Server returns results in batch of a fixed count (here 5000), so if we wait to get all blob properties it would take, and then processing of the blobs would take time further.
Lets say the number of blobs are very huge, it would result in OOM on the client.

For the above point, the pr introduces a logic of producer and consumer. Where, the producer would keep producing the blob properties on a separate thread and enqueue them in a queue. The consumer would be owning an executorservice, it would keep polling the queue, and keep submitting to the executorService. The pr would take care of the consumer lag should not be more than a configuration.

High level code design:

Whenever the handlers are executed, they call listRecursiveAndTakeAction to handler directory level operations.

Atomic Rename:
This would have to be implemented in BlobRenameHandler. This is required, because the rename over blob for a directory is not an O(1) operation in reference to the client, but it needs an orchestration to copy/delete each blob in the directory path. There is a need of atomic rename which is not switched on for every kind of directory, but there is a config in which the developer can provide what directory they want to enable for atomic rename. The config is fs.azure.atomic.rename.keywith default value is "/hbase" (on trunk in abfs). This feature is implemented in wasb for hbase case only. The need of why this feature was not implemented in abfs was that the dfs endpoint’s API call would be O(1). Since, we are bringing in blob endpoint integeration, this change has to be implemented.

What the feature is required to do:

Before start of rename of directory, it will take lease on the src directory, and before doing rename of each blob, it would take lease on those paths.
Before the rename of an subDir of an atomicDirectory, create a json in the parent of the subDir with path “parent/subDir-renamePending.json”. This would include the following:
a. subDirEtag
b. oldFolderName
c. newFolderName
Where would this json be used?
a. When doing listPath on a path gives the renamePending json as a subPath
b. When getPathStatus has to be done on the subDir of an atomcDirectory, client would also do getPathStatus on subDir-renamePending.json
What to do when json is found?
a. Rerun the rename as per the json
b. Once, rename orchestration is done, delete the json.

…Client

This reverts commit 0005ca9.

…ception from BlobRenaameHandler

lease arrayList a clone

… branch.

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

anmolanmol1234 · 2024-06-18T12:49:13Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-      throws IOException {
+      final FsPermission umask,
+      TracingContext tracingContext)
+      throws AzureBlobFileSystemException {


any specific reason for changing the exception type ?

anmolanmol1234 · 2024-06-18T12:53:19Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -1121,6 +1152,7 @@ public FileStatus getFileStatus(final Path path,
              path,
              isNamespaceEnabled);

+


increases git diff

anmolanmol1234 · 2024-06-18T12:57:38Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

+       * and resume that if it exists.
+       */
+      if (isAtomicRenameKey(path.toUri().getPath())) {
+        getClient().takeGetPathStatusAtomicRenameKeyAction(path, tracingContext);


Since this is a special handling only for blob client, can this be done in the client class ?

good point, although the action if its atomic key was being done in blobClient. But, now have exported the conditioning to the blobClient as well.

anmolanmol1234 · 2024-06-18T12:58:15Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-                          encryptionContext));
+          if (isAtomicRenameKey(entryPath.toUri().getPath())
+              && entryPath.toUri().getPath().endsWith(RenameAtomicity.SUFFIX)) {
+              getClient().takeListPathAtomicRenameKeyAction(entryPath, (int) contentLength,


Same for this scenario. Can this be handled in blob client class itself ?

good point, although the action if its atomic key was being done in blobClient. But, now have exported the conditioning to the blobClient as well.

anmolanmol1234 · 2024-06-18T12:59:18Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -1994,7 +2046,7 @@ public static final class Permissions {
    private final String permission;
    private final String umask;

-    Permissions(boolean isNamespaceEnabled, FsPermission permission,
+    public Permissions(boolean isNamespaceEnabled, FsPermission permission,


is this made public for some test usage ?

have been reverted.

anmolanmol1234 · 2024-06-19T06:35:45Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RenameAtomicity.java

+    AbfsRestOperation putBlobOp = abfsClient.createPath(path.toUri().getPath(),
+        true,
+        true, permissions, false, null, null, tracingContext);
+    String eTag = putBlobOp.getResult().getResponseHeader(


extractEtagHeader can be used

anmolanmol1234 · 2024-06-19T06:36:17Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RenameAtomicity.java

+
+    // PutBlock on the path.
+    byte[] blockIdByteArray = new byte[BLOCK_ID_LENGTH];
+    String blockId = new String(Base64.encodeBase64(blockIdByteArray),


add some unique string in the blockId

yes, would have a random byte array.

anmolanmol1234 · 2024-06-19T06:37:50Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RenameAtomicity.java

+  }
+
+  @VisibleForTesting
+  void createRenamePendingJson(Path path, byte[] bytes)


why explicit calls to create, append and flush, why not use fs.create which returns OuputStream and just call write, hysnc and flush on that ?

Thats what for I added the callbacks. We cannot have cycle of classes, right? I dont think client having a pointer to fileSystem would be good. What you feel?

anmolanmol1234 · 2024-06-19T06:38:55Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RenameAtomicity.java

+    // Make file contents as a string. Again, quote file names, escaping
+    // characters as appropriate.
+    String contents = "{\n"
+        + "  FormatVersion: \"1.0\",\n"


FormatVersion, OldFolderName etc should all come from Constants

Sure, will create a new class for serialization and deserialization to json string. That would standardize the json.

anmolanmol1234 · 2024-06-19T06:39:13Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/RenameAtomicity.java

+   * @return JSON string which represents the operation.
+   */
+  private String makeRenamePendingFileContents(String eTag) {
+    SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS");


Use constants

anujmodi2021

Added some thought...

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java

...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/extensions/SASTokenProvider.java

...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientContext.java

anujmodi2021 · 2024-06-19T06:32:36Z

...p-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java

@@ -921,8 +1006,8 @@ public AbfsRestOperation deletePath(final String path, final boolean recursive,
     */
    final List<AbfsHttpHeader> requestHeaders = (isPaginatedDelete(recursive,
        isNamespaceEnabled) && xMsVersion.compareTo(
-        ApiVersion.AUG_03_2023) < 0)
-        ? createDefaultHeaders(ApiVersion.AUG_03_2023)
+        AbfsHttpConstants.ApiVersion.AUG_03_2023) < 0)


I have imported AbfsConstatnt.ApiVersion
We don;t need to qualify it now.
Thoughts on this?

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ListActionTaker.java

anujmodi2021 · 2024-06-19T06:40:19Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobDeleteHandler.java

+      final boolean recursive,
+      final AbfsBlobClient abfsBlobClient,
+      final TracingContext tracingContext) {
+    super(path, abfsBlobClient, abfsBlobClient.getAbfsConfiguration()


We are already passing client, maybe we can get required configs from client itself in base class instead of passing them here.
Same for BlobRenameHandler as well.

It would be sending different config-keys. But, will simplify this. +1

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobRenameHandler.java

#120 * change exception type; other review comments; * comments wip * sync checks review wip * reviews comments at higher level taken * atomic method abstraction to client * Permissions not to have public constructor * added javadocs small fix : sync should return fix + lease can be sync and async. test fix3 fix2 fix1 * javadocs

deserialization needs empty constructor concurrency control to get availableSize no manual json parsing in renameAtomicity

…renameDelete

anujmodi2021 · 2024-06-24T10:16:42Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ListActionTaker.java

+      false);
+
+  public ListActionTaker(Path path,
+      AbfsClient abfsClient,


Sorry I missed this earlier, but looks like this should also be AbfsBlobClient

…BlobClient always. No casting required

anujmodi2021

Nits only

anujmodi2021 · 2024-06-24T10:58:21Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

  public void createDirectory(final Path path, final FsPermission permission,
-      final FsPermission umask, TracingContext tracingContext)
+      final FsPermission umask,


Nit: We can avoid this git diff. Line is not that long..

anujmodi2021 · 2024-06-24T10:58:38Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -1027,22 +1035,19 @@ public void breakLease(final Path path, final TracingContext tracingContext) thr
   * @param tracingContext trace context
   * @param sourceEtag etag of source file. may be null or empty
   * @throws AzureBlobFileSystemException failure, excluding any recovery from overload failures.
+   *


Reduce git diff

anujmodi2021 · 2024-06-24T10:58:54Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

   * @return true if recovery was needed and succeeded.
   */
  public boolean rename(final Path source,
      final Path destination,
      final TracingContext tracingContext,
-      final String sourceEtag) throws
-    IOException {
+      final String sourceEtag)


reduce git diff

anujmodi2021 · 2024-06-24T10:59:47Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

    final Instant startAggregate = abfsPerfTracker.getLatencyInstant();
    long countAggregate = 0;
    boolean shouldContinue;

-    if (isAtomicRenameKey(source.getName())) {


Why is this removed from DFS flow?? Was this not doing anything there??

anujmodi2021 · 2024-06-24T11:00:16Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -1081,7 +1096,7 @@ public boolean rename(final Path source,
  }

  public void delete(final Path path, final boolean recursive,
-      TracingContext tracingContext) throws AzureBlobFileSystemException {
+      TracingContext tracingContext) throws IOException {


Exception change needed?

anmolanmol1234 · 2024-06-25T10:42:35Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -720,7 +715,8 @@ private AbfsRestOperation conditionalCreateOverwriteFile(final String relativePa
          }
        }

-        String eTag = extractEtagHeader(op.getResult());
+        String eTag = op.getResult()


should be extractEtagHeader only right ?

anmolanmol1234 · 2024-06-25T10:45:20Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

@@ -1167,6 +1187,8 @@ public FileStatus getFileStatus(final Path path,

      perfInfo.registerSuccess(true);

+      getClient().takeGetPathStatusAtomicRenameKeyAction(path, tracingContext);


This function does not do any action for dfs, possible to move this also to blob endpoint ?

There is an expectation in blob that fs.getPathStatus and fs.listPath can resume a pending rename if the pending-json is found. True that there is no action required in dfs. As far as store is concerned, the implementation is abstracted out to client.

But, the reason why the invocation of the abstract method cannot be pushed down further in client.getFileStatus and client.listPath is that these functions get called in multiple place other than filesystem API call flows. But, as our expectation for resume to only happen is at filesystem api level, invoking the abstract method in store makes sense.

To push this behavior in the client, the client.getPathStatus and client.listPath would have to be overloaded to take a new arg which tells if the method is getting called from the API call flow or not, which I think would be antipattern, but open to your opinions on it.

anmolanmol1234 · 2024-06-25T10:45:45Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-                          entry.eTag(),
-                          encryptionContext));
+          final boolean actionTakenOnRenamePendingJson
+              = getClient().takeListPathAtomicRenameKeyAction(entryPath,


anmolanmol1234 · 2024-06-25T10:48:21Z

...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java

+      throw new AbfsRestOperationException(
+          AzureServiceErrorCode.PATH_NOT_FOUND.getStatusCode(),
+          AzureServiceErrorCode.PATH_NOT_FOUND.getErrorCode(),
+          "Path had to be recovered from atomic rename operation.",


exception message should come from constants class

anmolanmol1234 · 2024-06-25T10:56:50Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobRenameHandler.java

+        }
+      } finally {
+        if (srcAbfsLease != null) {
+          srcAbfsLease.cancelTimer();


release lease ?

good point. Made a small change here.

so, when the operation is successful -> src blob would be deleted. -> no need to release the lease. But, when failure happens -> we need to release. Good catch!

now, when successful -> cancel timer
when failure -> release lease + cancel timer -> done via free method.

anmolanmol1234 · 2024-06-25T14:52:58Z

LGTM. Thanks for taking the suggestions.

saxenapranav added 30 commits May 6, 2024 05:11

RenameHandler + DfsRenameHandler

c036404

pathinformation; shorten prechecks for dfs case

ea2bef1

prechecks for blob handler

870361b

deleteHandler

c2bacc3

new config: fs.azure.blob.implicit.check.enabled

0005ca9

Merge branch 'implicitConfg' into renameDelete

c3372af

Merge branch 'azureBlobClient' into renameDelete

75edc7b

ListActionTaker

f17b0b7

delete code done

eb642bc

wip

798e23e

copy progress

8282cc4

copy Blob impl

fe41cca

renameAtomicity

0ad1214

listStatus and getPathStatus to do renameAtomic

4d48fe8

test fixture

0b9b032

Merge branch 'azureBlobClient' into renameDelete

4ffef8f

createPath, getBlobProperty in blob for meanwhile; to be removed later

d5acd2f

Merge branch 'azureBlobClient' into renameDelete

710ca79

no renameHandler abstract class; have blobRenameHandler from the blob…

df84b40

…Client

deleteHandler

2c67654

rename and delete internal method

e030dc7

remove unnecessary class

bab4bb8

createCallback and readCallback in store

0fc7ca8

Revert "new config: fs.azure.blob.implicit.check.enabled"

ceb2dc7

This reverts commit 0005ca9.

important changes for renameatomicity

5661eb2

abfsbloblease in renamehandler

1b3ee3f

client forwards; tests additions;

97b6b03

all current delete tests: good.

ca6feac

rename required changes to make test run

f96afce

call pathStatus on nonRoot path; is src path is not there, throw IOEx…

ce181de

…ception from BlobRenaameHandler

saxenapranav added 3 commits June 13, 2024 00:22

take action on renamePending json from client abstract method

182967e

getClient to get mocking correct in the test

a705138

lease arrayList a clone

to return BLOB_PATH_NOT_FOUND is already added in the azureBlobClient…

736b32a

… branch.

anujmodi2021 reviewed Jun 14, 2024

View reviewed changes

saxenapranav added 6 commits June 13, 2024 23:27

review comments

652c15b

removed callbacks; test fixes;

5303166

added asf and javadocs on RenameAtomicityTestUtils

ec869ee

remove overwrite overload

6f26b81

javadocs

8c5af2e

Merge branch 'azureBlobClient' into renameDelete

ebde8d9

anmolanmol1234 requested changes Jun 19, 2024

View reviewed changes

anujmodi2021 requested changes Jun 19, 2024

View reviewed changes

saxenapranav changed the base branch from azureBlobClient to wasbDepCodeReview June 19, 2024 10:14

saxenapranav and others added 5 commits June 21, 2024 17:16

Merge branch 'wasbDepCodeReview' into renameDelete

5f92b37

consume JsonProcessingException if json string is invalid

8cb4c6f

deserialization needs empty constructor concurrency control to get availableSize no manual json parsing in renameAtomicity

Merge branch 'renameDelete' of github.com:ABFSDriver/AbfsHadoop into …

dd6f5dc

…renameDelete

import checks

5afeaef

anujmodi2021 reviewed Jun 24, 2024

View reviewed changes

remove casting from ListActionTaker for client, it s going to be Abfs…

e816e96

…BlobClient always. No casting required

anujmodi2021 approved these changes Jun 24, 2024

View reviewed changes

nits refactors;

7b53575

anmolanmol1234 requested changes Jun 25, 2024

View reviewed changes

saxenapranav added 3 commits June 25, 2024 04:50

Merge branch 'wasbDepCodeReview' into renameDelete

d6de0c4

refactors

603caea

release the lease when the rename is not fully successful

52bcd11

anmolanmol1234 approved these changes Jun 25, 2024

View reviewed changes

saxenapranav merged commit d078fb1 into wasbDepCodeReview Jun 26, 2024
1 check passed

saxenapranav mentioned this pull request Jul 1, 2024

Remove atomic dir action in fileStatus and listStatus from store class #123

Merged

		@@ -1121,6 +1152,7 @@ public FileStatus getFileStatus(final Path path,
		path,
		isNamespaceEnabled);

		@@ -1167,6 +1187,8 @@ public FileStatus getFileStatus(final Path path,

		perfInfo.registerSuccess(true);

		getClient().takeGetPathStatusAtomicRenameKeyAction(path, tracingContext);

Rename Delete on Blob endpoint. #117

Rename Delete on Blob endpoint. #117

Conversation

saxenapranav commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saxenapranav Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saxenapranav Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anujmodi2021 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anujmodi2021 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolanmol1234 commented Jun 25, 2024

saxenapranav Jun 21, 2024 •

edited

Loading

saxenapranav Jun 21, 2024 •

edited

Loading