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-11661. ITestS3AContractBulkDelete fails with FSO bucket #7572

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

chungen0126
Copy link
Contributor

@chungen0126 chungen0126 commented Dec 13, 2024

What changes were proposed in this pull request?

New contract test ITestS3AContractBulkDelete (added by HADOOP-18679 in Hadoop 3.4.1) is failing with FSO bucket.
Bulk delete doesn't delete directories. Therefore, It passed path without trailing slash. In the original implementation, directories would be deleted if paths were passed without trailing slashes.

To fix this, delete the directory only if the trailing slash present in the path on the server side. Also, make sure that directory paths are passed with trailing slashes on the client side.

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

How was this patch tested?

CI: https://github.com/chungen0126/ozone/actions/runs/12308665244

@chungen0126 chungen0126 marked this pull request as ready for review December 13, 2024 13:05
@adoroszlai
Copy link
Contributor

Thanks @chungen0126 for the patch, and the description of the problem.

To fix this, delete the directory only if the trailing slash present in the path on the server side. Also, make sure that directory paths are passed with trailing slashes on the client side.

I think we should consider client-server compatibility across versions. What if old client sends the request, without /, to new server, which expects /?

Current xcompat tests probably do not cover this operation.

@chungen0126 chungen0126 marked this pull request as draft December 13, 2024 18:41
@chungen0126 chungen0126 marked this pull request as ready for review December 13, 2024 23:08
@chungen0126
Copy link
Contributor Author

chungen0126 commented Dec 13, 2024

@adoroszlai Thanks for your review.

I think we should consider client-server compatibility across versions. What if old client sends the request, without /, to new server, which expects /?

That sounds like a problem. I am adding a new argument for requests from S3 to let the server know where the requests are coming from and only check the requests from S3. This might solve the problem. Do you think we need to add a test case in the xcompat tests for this?

@adoroszlai
Copy link
Contributor

I am adding a new argument for requests from S3 to let the server know where the requests are coming from and only check the requests from S3

Instead of adding the flag, please use ClientVersion, which is already sent in requests. Add a new constant and check for it in OM, like:

if (ClientVersion.fromProtoValue(req.getVersion())
.compareTo(ClientVersion.BUCKET_LAYOUT_SUPPORT) < 0) {

Do you think we need to add a test case in the xcompat tests for this?

Yes, please.

@adoroszlai adoroszlai marked this pull request as draft December 14, 2024 05:17
@errose28
Copy link
Contributor

I think adding new client versions is still blocked on #6932 or else they will not be correctly validated cc @swamirishi

@adoroszlai
Copy link
Contributor

adoroszlai commented Dec 16, 2024

I think adding new client versions is still blocked on #6932 or else they will not be correctly validated

Nice catch. We need to get that one fixed. This contract test can wait.

@chungen0126
Copy link
Contributor Author

chungen0126 commented Dec 16, 2024

I think adding new client versions is still blocked on #6932 or else they will not be correctly validated cc @swamirishi

Thanks @errose28 for the mentioning it. I actually encounter that problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants