HADOOP-19654. Upgrade AWS SDK to 2.32.23 #7882
Draft
+306
−63
Conversation
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
I found |
|
@pan3793 maybe. what is unrelated is out the box the SDK doesn't do bulk delete with third party stores which support it (Dell ECS). |
|
@pan3793 no, it's lifecycle related. Test needs to set up that minicluster before the test cases. and that's somehow not happening |
steveloughran
force-pushed
the
s3/HADOOP-19654-aws-sdk-2.32
branch
from
5b9a7e3 to
efd34a0
Compare
|
regressions everywhereNo logging. Instead we get
more on this once I've looked at it. If it is an SDK issue, major regression, though it may be something needing changes in the aal libary s3 expressassumption: now that the store has lifecycle rules, you don't get prefix listings when there's an in-progress upload. Fix: change test but also path capability warning of inconsistency. this is good. Operation costs/auditing count an extra HTTP request, so cost tests fail. I suspect it is always calling CreateSession, but without logging can't be sure |
steveloughran
force-pushed
the
s3/HADOOP-19654-aws-sdk-2.32
branch
from
efd34a0 to
6a7e6d9
Compare
steveloughran
force-pushed
the
s3/HADOOP-19654-aws-sdk-2.32
branch
from
6a7e6d9 to
cc31e5b
Compare
|
💔 -1 overall
This message was automatically generated. |
4 tasks
steveloughran
force-pushed
the
s3/HADOOP-19654-aws-sdk-2.32
branch
from
cc31e5b to
3351e41
Compare
|
Thanks @steveloughran, PR looks good overall. Are then failures in |
ahmarsuhail
reviewed
Sep 16, 2025
| // disable create session so there's no need to | ||
| // add a role policy for it. | ||
| disableCreateSession(conf); | ||
| //disableCreateSession(conf); |
There was a problem hiding this comment.
nit: can just cut this instead of commenting it out, since we're skipping these tests if S3 Express is enabled
| // close the stream, should throw RemoteFileChangedException | ||
| RemoteFileChangedException exception = intercept(RemoteFileChangedException.class, stream::close); | ||
| assertS3ExceptionStatusCode(SC_412_PRECONDITION_FAILED, exception); | ||
| verifyS3ExceptionStatusCode(SC_412_PRECONDITION_FAILED, exception); |
There was a problem hiding this comment.
do you know what the difference is with the other tests here?
As in, why with S3 express is it ok to assert that we'll get a 412, whereas the others tests will throw a 200?
There was a problem hiding this comment.
Hey, it's your server code. Go see.
|
💔 -1 overall
This message was automatically generated. |
|
I've attached a log of a test run against an s3 express bucket where the test The relevant stuff is at line 564 where a HEAD request fails because the stream is broken The second request always works. Either the request is being rejected (why?) or the connection has gone stale. But why should it happen at exactly the same place on every single test run? org.apache.hadoop.fs.s3a.statistics.ITestAWSStatisticCollection-output.txt |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran discovered completely by accident, but it's something to do with the checksumming code. If you comment out these lines: the test will pass. Could be something to do with s3Express not supporting md5, will look into it. |
|
Specifically, it's this line: Comment that out, or change it to My guess is it's something to do with S3 express not supporting MD5, but for operations where Have asked the SDK team. |
|
ok, so maybe for s3express stores we don't do legacy MD5 plugin stuff all is good?
While on the topic of S3 Express, is it now the case that because there's lifecycle rules for cleanup, LIST calls don't return prefixes of paths with incomplete uploads? If so I will need to change production code and the test -with a separate JIRA for that for completeness |
@steveloughran confirming with the SDK team, since the MD5 plugin is supposed to restore previous behaviour, the server rejecting the first request seems wrong. let's see what they have to say.
Will check with S3 express team on this |
This is just the library declaration change.
* create legacy MD5 headers * downgrade request checksum calculation to "when required" * set the (existing) checksum validation flag the same way so the builder library is happy * add "none" as a valid checksum algorithm * document that and the existing "unknown_to_sdk_version" option * with troubleshooting * ITestS3AChecksum modified to handle these checksum changes.
… of multiparts is 200 + error * Recognise and wrap with RemoteFileChangedException * Fix up assertions in test.
Add relevant statements for s3 access and skip all tests which expect partial access to paths on a bucket.
...so adds cost to the assertion
…3 express Revert test to original assert (no special treatment of s3 express), and log Initiating GET request" for ease of log scanning. Add to the log4.properties file the settings needed to turn on wire logging, commented out by default.
steveloughran
force-pushed
the
s3/HADOOP-19654-aws-sdk-2.32
branch
from
661dc6e to
aa8e814
Compare
|
thanks. I don't see it on tests against s3 with the 2.29.52 release, so something is changing with the requests made with new SDK + MD5 stuff. |
|
@steveloughran not able to narrow this error down just yet, it looks like it's a combination of S3A's configuration of the S3 client + these new Md5 changes. I see the failure when the S3A client, and don't see it when I use a newly created client. So it's not just because of Looking into it some more. S3 express team said there have been no changes in LIST behaviour. |
|
able to reproduce the issue outside of S3A. Basically did what would happen when you run a test in S3A:
The head fails, but if you comment out no idea what's going on. but have shared this local reproduction with SDK team. And rules out that it's something in the S3A code. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
How was this patch tested?
Testing in progress; still trying to get the ITests working.
JUnit5 update complicates things here, as it highlights that minicluster tests aren't working.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?