-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19485. S3A: upgrade AWS SDK to 2.29.52 #7479
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
HADOOP-19485. S3A: upgrade AWS SDK to 2.29.52 #7479
Conversation
|
testing in progress; also writing a new, expanded and very strict doc on qualifying a release, based on the experience of recent upgrades. |
|
💔 -1 overall
This message was automatically generated. |
|
Test failures with the 2.30.27 The issue of HADOOP-19272 #7048; S3A: AWS SDK 2.25.53 warnings logged about transfer manager not using CRT client has been fixed this test can be culled |
8639127 to
9720b02
Compare
|
💔 -1 overall
This message was automatically generated. |
|
|
|
Testing with third party stores shows the MD-5 thing is there. Also shows that no SDK testing is ever performed against third-party stores, which is something to consider |
9720b02 to
b8314e3
Compare
|
💔 -1 overall
This message was automatically generated. |
b8314e3 to
288fd09
Compare
| * How far in advance of credential expiry must IAM credentials be refreshed. | ||
| * See HADOOP-19181. S3A: IAMCredentialsProvider throttling results in AWS auth failures | ||
| */ | ||
| public static final Duration TIME_BEFORE_EXPIRY = Duration.ofMinutes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it good to have this configurable ?
| public static final String EXCEPTION_THROWN_BY_INTERCEPTOR = "Exception thrown by interceptor"; | ||
|
|
||
| /** | ||
| * Text to include in asseertions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: assertions
|
@steveloughran - I can help you test with S3 express storage bucket for this change. Please do let me know once everything is ready |
|
going to have to go with a 2.29 release; those 2.30 releases brake 3rd party stores and I'm not going to get into the work of trying to fix that before a release. And although the stance of SDK dev team is
My stance is: they just broke everything. Unless they want downstream apps to stay on 2.29 for a long time -they need to restore the old behaviour. I've tried to get their recommended workaround in -but it doesn't fix the tests failures. |
288fd09 to
1900c64
Compare
|
latest PR updates to 2.29.52; avoids all the new checksum stuff -but also means that IAM throttling fix isn't in either. Pulled that out. chain now has
That's all. |
|
Output of looking at the output of a branch-3.4 release they are not new; this not a regression, |
1900c64 to
fef747c
Compare
|
@shameersss1 would love that s3-express stuff, and any manual testing you can do. The ILoad* tests against that and s3standard from within aws infra would be good too -I'm now suspecting the SDK doesn't escalate a 503 from within a bulk delete response into a 503 for the whole request -so nothing is retrying it. Here's my WiP design of a new test process, and yes it is strict and time consuming. I already think it's too many buckets to be realistically tested -but we do need that mix. Please do as much as you can. And I'll add "seek help!" to the doc -not just to save time but to ensure even broader test setups. |
|
💔 -1 overall
This message was automatically generated. |
|
Will need to re-base. Note that when I merge this, I will split it into three patches |
|
💔 -1 overall
This message was automatically generated. |
|
FYI all good with the 2.29 upgrade against aws and 2 external stores; https://issues.apache.org/jira/browse/HADOOP-19512 highlights issues -but those all seem unrelated. The main bit remaining is dependency audit. |
cnauroth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I just entered one question about a comment.
| .serviceConfiguration(serviceConfiguration); | ||
|
|
||
| if (LOG.isTraceEnabled()) { | ||
| // if this log is set to debug then we turn on logging of SDK metrics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to say "...set to trace.."? The if statement is looking for trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. let me change the doc; I tried with debug first but it was waay too noisy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chris; fixed this..pushed up a whole new pr as I have the chain of patches to merge independently...I've added this and the yetus complaints to the relevant commits
|
💔 -1 overall
This message was automatically generated. |
3ca156e to
26bca7c
Compare
|
💔 -1 overall
This message was automatically generated. |
|
yarn is in the process of upgrading tests to junit 5; consider these failures unrelated |
cnauroth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, @steveloughran . Just one more nitpick, again about comment text. Please assume +1 from me after addressing that. (No need for another review round.)
| # services it launches itself. | ||
| # log4.logger.org.apache.hadoop.service=DEBUG | ||
|
|
||
| # log this at trace to trigger enabling the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there more text intended here? e.g. "...enabling the AWS SDK metrics logging."
| ``` | ||
|
|
||
| ```text | ||
| INFO metrics.LoggingMetricPublisher (LoggerAdapter.java:info(165)) - Metrics published: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: is this logged on every request, or at some fixed interval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, it's noisy. It's the ultimate way to see what is happening in the SDK. I'd been meaning to do this for a while, but trying to see what the 2.30 SDK was up to motivated me.
It is very much emergency use only -those situations where you turn on all of the http traffic logging. But it is now avaiable for those emergencies
ahmarsuhail
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM
|
💔 -1 overall
This message was automatically generated. |
|
thanks for approval, I will manually merge in very carefully |
This is the latest release of the AWS SDK which does not make changes that are incompatible with third-party stores. Any followup will require addressing HADOOP-19490. Change-Id: I51bccc4376681b6b3058060b5aa8af9730886e12
Code changes related to HADOOP-19485. AwsSdkWorkarounds no longer needs to cut back on transfer manager logging (HADOOP-19272). - Remove log downgrade and change assertion to expect nothing to be logged. - remove false positives from log ITestS3AEndpointRegion failure: Change in state of AwsExecutionAttribute.ENDPOINT_OVERRIDDEN attribute requires test tuning to match. Change-Id: Iee88e13c113daf852c82c4a11c5029a62efabb71
Switch is in client; commented out in test log properties; covered in troubleshooting doc Change-Id: If70447d8eb3d3d0e03db5c169cd1aabf844931bd
* Includes fix for the assumeStoreAwsHosted() logic. * Documents how to turn off multipart uploads with third-party stores Change-Id: Iae344b372dceaca981426035e062b542af25f0cd
…tore. Downgrade warning message from INFO to DEBUG. Change-Id: Ibb7be344e44e2d66d779c40bf086edba5ce7d7d8
1664b94 to
aebf99e
Compare
|
thanks @steveloughran , once this is in could you also give the CRT PR another look. Can cut the 3.4.2 branch by the end of the week hopefully, once these are in. What did you decide about conditional writes: #7011 , do we want to wait for that? |
|
manually merged |
|
@ahmarsuhail will look at the CRT, but still getting this chain backported. All good except for HADOOP-19527 -which now seems to happen everywhere. w.r.t conditional -plan for end of week. |
This upgrades to the final release the AWS SDK which retains compatibility with third-party stores. Note: this patch MUST be followed by * HADOOP-19485. S3A: Test failures after SDK upgrade And SHOULD be followed up with * HADOOP-19455. S3A: SDK client to log metrics at TRACE * HADOOP-19516. S3A: SDK reads content twice during PUT to S3 Express store. Contributed by Steve Loughran.
|
💔 -1 overall
This message was automatically generated. |
This upgrades to the final release the AWS SDK which retains compatibility with third-party stores. Note: this patch MUST be followed by * HADOOP-19485. S3A: Test failures after SDK upgrade And SHOULD be followed up with * HADOOP-19455. S3A: SDK client to log metrics at TRACE * HADOOP-19516. S3A: SDK reads content twice during PUT to S3 Express store. Contributed by Steve Loughran.
This upgrades to the final release the AWS SDK which retains compatibility with third-party stores. Note: this patch MUST be followed by * HADOOP-19485. S3A: Test failures after SDK upgrade And SHOULD be followed up with * HADOOP-19455. S3A: SDK client to log metrics at TRACE * HADOOP-19516. S3A: SDK reads content twice during PUT to S3 Express store. Contributed by Steve Loughran.
### What changes were proposed in this pull request? This PR aims to upgrade AWS SDK v2 to 2.29.52. ### Why are the changes needed? Like [Apache Iceberg v1.8.1](https://iceberg.apache.org/releases/#181-release) and Apache Hadoop 3.4.2 (HADOOP-19485), Apache Spark 4.1.0 had better use the latest one. - apache/hadoop#7479 - apache/iceberg#12339 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50731 from dongjoon-hyun/SPARK-51929. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Kent Yao <yao@apache.org>
### What changes were proposed in this pull request? This PR aims to upgrade AWS SDK v2 to 2.29.52. ### Why are the changes needed? Like [Apache Iceberg v1.8.1](https://iceberg.apache.org/releases/#181-release) and Apache Hadoop 3.4.2 (HADOOP-19485), Apache Spark 4.1.0 had better use the latest one. - apache/hadoop#7479 - apache/iceberg#12339 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50731 from dongjoon-hyun/SPARK-51929. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Kent Yao <yao@apache.org>
Upgrade to 2.29.52 -the last version compatible with third party stores until there are fixes in the AWS SDK or workarounds added in the S3A connector
How was this patch tested?
Regression testing in progress
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?