Skip to content
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

HDDS-10572. Implement multiDelete using OMKeysDeleteRequest #6751

Merged
merged 13 commits into from
Jun 7, 2024

Conversation

Tejaskriya
Copy link
Contributor

@Tejaskriya Tejaskriya commented May 30, 2024

What changes were proposed in this pull request?

BucketEndpoint#multiDelete deletes keys one by one. This causes 2 problems:

  1. Bad performance: To delete each key, a round trip to the OM is made.
  2. DIRECTORY_NOT_EMPTY errors, due to deletion order (in FSO buckets): For FSO buckets, if the directory is mentioned before the keys(inside the directory) in the multidelete request, it causes the request to fail even though it should be successful.

By reimplementing multidelete using OMKeysDeleteRequest to delete all items in a batch, these problems can be resolved.
With the existing API deleteKeys, we would not be able to get a list of errors for the keys that could not be deleted. Moreover, a OMException was being thrown if any errors were encountered.
In this PR, a new API is introduced deleteKeysQuiet to tackle these changes. It returns the list of errors without throwing an exception in case of any errors. This API is used by multidelete in the S3 gateway's BucketEndpoint#multiDelete

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10572

How was this patch tested?

Existing tests and locally in docker setup cluster.

$ aws s3 ls --endpoint http://localhost:9878 --recursive s3://bfso  
2024-05-31 12:38:53          0 dir1/
2024-05-31 12:38:53          0 dir1/dir2/
2024-05-31 12:38:54          3 dir1/dir2/k1
2024-05-17 13:13:54          3 k2

$ aws s3api delete-objects --endpoint http://localhost:9878 --delete file://delete.json --bucket bfso 
{
    "Deleted": [
        {
            "Key": "dir1"
        },
        {
            "Key": "dir1/dir2"
        },
        {
            "Key": "dir1/dir2/k1"
        }
    ]
}

$ aws s3 ls --endpoint http://localhost:9878 --recursive s3://bfso   
2024-05-17 13:13:54          3 k2


$ aws s3 ls --endpoint http://localhost:9878 --recursive s3://bos    
2024-05-31 12:38:45         3 dir1/dir2/k1
2024-05-17 13:14:13          3 k2

$ aws s3api delete-objects --endpoint http://localhost:9878 --delete file://delete.json --bucket bos   
{
    "Deleted": [
        {
            "Key": "dir1"
        },
        {
            "Key": "dir1/dir2"
        },
        {
            "Key": "dir1/dir2/k1"
        }
    ]
}

$ aws s3 ls --endpoint http://localhost:9878 --recursive s3://bos    
2024-05-17 13:14:13          3 k2

@Tejaskriya Tejaskriya marked this pull request as ready for review May 31, 2024 08:25
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tejaskriya for the patch. The implementation works fine. I have some suggestion for the new API, and some nits.

Also, please enable ITestS3AContractMkdir, which was disabled due to the inferior existing multiDelete implementation:

diff --git hadoop-ozone/dist/src/main/compose/common/s3a-test.sh hadoop-ozone/dist/src/main/compose/common/s3a-test.sh
index 1a784b1161..554b22b5a3 100644
--- hadoop-ozone/dist/src/main/compose/common/s3a-test.sh
+++ hadoop-ozone/dist/src/main/compose/common/s3a-test.sh
@@ -95,10 +95,9 @@ EOF
   # Some tests are skipped due to known issues.
   # - ITestS3AContractDistCp: HDDS-10616
   # - ITestS3AContractGetFileStatusV1List: HDDS-10617
-  # - ITestS3AContractMkdir: HDDS-10572
   # - ITestS3AContractRename: HDDS-10665
   mvn -B -V --fail-never --no-transfer-progress \
-    -Dtest='ITestS3AContract*, ITestS3ACommitterMRJob, !ITestS3AContractDistCp, !ITestS3AContractGetFileStatusV1List, !ITestS3AContractMkdir, !ITestS3AContractRename' \
+    -Dtest='ITestS3AContract*, ITestS3ACommitterMRJob, !ITestS3AContractDistCp, !ITestS3AContractGetFileStatusV1List, !ITestS3AContractRename' \
     clean test
 
   local target="${RESULT_DIR}/junit/${bucket}/target"

@adoroszlai adoroszlai added the s3 S3 Gateway label Jun 2, 2024
@Tejaskriya
Copy link
Contributor Author

@adoroszlai Thank you for the review! I have enabled the S3A contract tests and addressed all your comments. Could you please take another look at the updated patch?

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tejaskriya Thanks for the patch.

@Tejaskriya
Copy link
Contributor Author

@ashishkumar50 Thanks for the review! I have addressed the review comments. Could you please review the new patch?

@ashishkumar50
Copy link
Contributor

@Tejaskriya Thanks for updating patch, LGTM.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tejaskriya for updating the patch.

@Tejaskriya
Copy link
Contributor Author

@adoroszlai thanks for the review and approval! I took a look at the CI failures and they don't seem to be related to the PR changes. They execute successfully locally. Could you please re-trigger them?

@adoroszlai adoroszlai merged commit 4b8767a into apache:master Jun 7, 2024
40 checks passed
@adoroszlai
Copy link
Contributor

Thanks @Tejaskriya for the patch, @ashishkumar50 for the review.

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jun 17, 2024
)

(cherry picked from commit 4b8767a)

 Conflicts:
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OmKeysDeleteRequestWithFSO.java

Change-Id: If4e71744048b0191b8fe33f5dd08027e2b550902
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jun 17, 2024
)

(cherry picked from commit 4b8767a)

 Conflicts:
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OmKeysDeleteRequestWithFSO.java

Change-Id: If4e71744048b0191b8fe33f5dd08027e2b550902
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s3 S3 Gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants