Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Thanks for looking into this feature! Let's merge my PR into yours: deepujain#1 |
Contributor
Author
|
@EnricoMi Sure. Once you’re done, please let me know if anything is pending from my side. I’m not familiar with the process or timeline yet, as this is my first time contributing to the Hadoop projects. I currently have a few pending PRs awaiting merge. |
Contributor
Author
|
My JIRA id is deepujain |
Contributor
|
Great! If you are happy with my proposed changes to your PR, merge deepujain#1. |
Contributor
Author
|
Merged your PR, thanks for the docs and rephrase. @EnricoMi |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Mar 13, 2026
steveloughran
left a comment
steveloughran
left a comment
Contributor
There was a problem hiding this comment.
all good, one minor checkstyle change and I'm happy.
Presumably you've tested this against a store with the feature?
Contributor
Author
|
Checkstyle fix is in (split the long LOG.debug line in DeleteOperation.java). Yes : tested against a VAST endpoint with the feature; enabled the option and confirmed recursive delete uses a single delete request. Thanks for the review. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
JIRA id is deepujain |
EnricoMi
suggested changes
Apr 10, 2026
Contributor
|
@deepujain how did you test this? Did you run Did you
With 1., obviously, assertions in tests With 1. and 2., non-delete requests fail for me with This is against VAST 5.4.3.1. |
Contributor
|
Adding this to shows request headers: |
Contributor
|
Maybe we also have to adjust the instrumentation: EnricoMi#2 At least we should make this debug log message correct when |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
April 13, 2026 23:43
9f2d9dd to
27eacbb
Compare
Contributor
Author
|
Addressed the current review feedback and force-pushed the branch. The optimized non-empty-directory path now sends |
|
🎊 +1 overall
This message was automatically generated. |
EnricoMi
reviewed
Apr 14, 2026
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
April 14, 2026 14:48
27eacbb to
1426ae3
Compare
Contributor
Author
|
Addressed the new review point and force-pushed. This revision removes the hardcoded proprietary delete header from S3A, adds generic request-type custom header support (fs.s3a.client.{s3,sts}.custom.headers.request.REQUEST), and updates the non-empty-directory delete docs to use request-scoped configuration when an endpoint needs extra headers. Local validation passed with JAVA_HOME=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home/bin:$PATH /opt/homebrew/bin/mvn -Dmaven.repo.local=/tmp/codex-m2 test -pl hadoop-tools/hadoop-aws -am -Dtest=TestAwsClientConfig,TestRequestFactory -DskipTests=false (20 tests, 0 failures). |
|
💔 -1 overall
This message was automatically generated. |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
April 21, 2026 18:04
1426ae3 to
b530128
Compare
|
💔 -1 overall
This message was automatically generated. |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
April 22, 2026 01:18
b530128 to
619773b
Compare
Contributor
Author
|
Follow-up push for the latest javadoc artifact: the remaining new warning was on CUSTOM_REQUEST_HEADERS_S3_PREFIX because the added @value tag is not valid on that derived String constant. I removed that tag and force-pushed the updated single commit. The exact module-level javadoc command is still not reproducible locally here because standalone hadoop-aws resolution needs snapshot artifacts that are not available from apache.snapshots in this environment, but the targeted AWS tests from the earlier push still passed locally: TestAwsClientConfig and TestRequestFactory (20 tests, 0 failures). |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
This PR is currently doing two things: skip recursive file deletion (configurable) and allow for custom per-request headers. This should be split and reviewed in two separate PRs, this one and #8417. |
Contributor
Author
|
Rebased this onto current apache/trunk, squashed it back to a single commit, and split out the generic per-request-header work so this PR now only covers the non-empty-directory delete behavior. Local validation passed with |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
June 15, 2026 07:30
619773b to
fed106d
Compare
deepujain
changed the title
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
The scope split is now done and CI is green on the current head. @EnricoMi, when you have a moment, could you please take another look? Happy to make any follow-up changes. |
Contributor
Author
|
The scope split is done and CI is still green on the current head. @EnricoMi, when you have a moment, could you please take another look? Happy to make any follow-up changes. |
Contributor
Author
|
The scope split is done, CI is still green on the current head, and I don’t believe anything else is pending from my side right now. @EnricoMi, when you have a moment, could you please take another look? Happy to make any follow-up changes. |
Contributor
Author
|
The scope split is done, CI is still green on the current head, and I still do not believe anything else is pending from my side as of July 20, 2026. @EnricoMi, when you have a moment, could you please take another look? Happy to make any follow-up changes. |
Contributor
Author
|
No code changes here. The scope split is done, this branch still only covers the non-empty-directory delete behavior, and CI is green on the current head as of July 26, 2026. @EnricoMi, when you have a moment, could you please take another look and let me know if anything is still pending from my side? |
Contributor
Author
|
The current branch now only covers the non-empty-directory delete behavior; the generic header work stays in #8417, and the earlier test-structure/config follow-ups are addressed in the current diff. CI is green on the current head. @EnricoMi @steveloughran, when you have a moment, could you please take another look and let me know if anything is still pending from my side? |
Contributor
Author
Contributor
Author
|
Following up on the August 15 question: the current branch remains limited to the non-empty-directory delete behavior, while the generic header work is in #8417. @EnricoMi, if an earlier review item still needs a change, please let me know; otherwise, could you re-review the current diff when convenient? |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
September 1, 2026 16:05
fed106d to
dc2e4f0
Compare
Contributor
|
ℹ️ Cloud-AWS manual trigger instructions: S3A tests must be manually triggered for fork pull requests. A maintainer should:
fork head: |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
Hi @steveloughran @ZanderXu @slfan1989, HADOOP-19801 adds an opt-in flag to skip recursive deletes on non-empty S3A dirs. Open since March, rebased on trunk with contract tests. Would love a sanity check on the S3A semantics before another month goes by. |
Contributor
Author
|
Split the long javadoc @PARAM line and the deleteDirectoryTree debug log at the comma per checkstyle. Capability tests already extend AbstractS3ATestBase (not AbstractContractDeleteTest) and use an isolated Configuration with removeBaseAndBucketOverrides for the option key. Ready for re-review. |
|
🎊 +1 overall
This message was automatically generated. |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
September 3, 2026 17:59
710f3b0 to
cd153a7
Compare
|
🎊 +1 overall
This message was automatically generated. |
deepujain
force-pushed
the
HADOOP-19801-delete-non-empty-directory
branch
from
September 4, 2026 15:58
cd153a7 to
9986462
Compare
Contributor
Author
|
Rebased cleanly onto current trunk at |
|
🎊 +1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
fs.s3a.delete.non-empty-directory.enabledso S3A can use a single delete request for a non-empty directory key instead of recursively listing and deleting child objects first. This PR is now scoped only to that delete behavior for S3-compatible endpoints that support it.Change
fs.s3a.delete.non-empty-directory.enabledand its default.hasPathCapability().Evidence it works
JAVA_HOME=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home ./mvnw -Dmaven.repo.local=/tmp/codex-m2 test -pl hadoop-tools/hadoop-aws -am -Dtest=TestAwsClientConfig -DskipTests=falseTestAwsClientConfig, 9 tests, 0 failures).JIRA
Fixes HADOOP-19801