-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-19282. STSClientFactory: do not use URIBuilder #7483
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-19282. STSClientFactory: do not use URIBuilder #7483
Conversation
URIBuilder was used from the AWS SDK for Java v2, to be precise from the shaded Apache HTTP Client. It is a problem if a user would like not to use the AWS SDK bundle, since more or less only 3 modules are needed (s3, s3-transfer & sts), but that may cause problems on unshaded dependency versions. Since a URI constructor can achieve the same here I switched it as a preferred option.
|
💔 -1 overall
This message was automatically generated. |
|
Hm. Looks like CI run wasn't very good: the only error I found in linked logs is Maven running out of threads/memory. Do I need to do something? |
|
Hi. I'm also interested in this change. |
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
|
Hey! I was expecting maybe somebody would take a look and see that my code doesn't cause failures. Oh well, will probably have to investigate CI myself. |
|
@LDVSOFT reopen it; we've just turned on auto-delete and a lot of my own WiP PRs are gone too! |
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.
code looks good.
@LDVSOFT can you re-open this, address my comments about imports and push out again, this should trigger things again.
we don't want to force down the full aws sdk on everyone even if it is the only way to avoid classpath conflict in big deployments
| import java.net.URISyntaxException; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.slf4j.Logger; |
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.
prefer these left as they were so the diff is smaller; import reordering makes cherrypicking a PITA
Description of PR
URIBuilderwas used from the AWS SDK for Java v2, to be precise from the shaded Apache HTTP Client. It is a problem if a user would like not to use the AWS SDK bundle, since more or less only 3 modules are needed (s3, s3-transfer & sts), but that may cause problems on unshaded dependency versions. Since a URI constructor can achieve the same here I switched it as a preferred option.How was this patch tested?
I've run the test suite against a eu-west-1 bucket, without scaling/load since the change shouldn't affect that. To be exact, with something like this:
auth-keys.xmlAlmost all test pass:
ITestDelegatedMRJobwork. They probably clean out environment somewhere and my environment-provided AWS credentials didn't work. Also it looks parametrized, and I can't tell from the Surefire/Failsafe reports which causes a problem.ITestRoleDelegationInFilesystem/ITestSessionDelegationInFilesystemfail a bit inmissmatch2, but I'm really unfamiliar with credentials delegation. Probably lost environment variables on it's way?ITestS3APrefetchingInputStreamfails with 0 size.Given that other tests pass and the scope of the change I think it's fine, and the problem is my test setup misconfiguration. If you know how to fix the setup — I can rerun with some other options.
Also, I've found this bug while repackaging Spark for a local K8S deployment, and with this fix STS configuration options work even if I do replace AWS SDK bundle with only required SDK modules.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?Sign-off
I give a license to the Apache Software Foundation to use this code, as required under §5 of the Apache License.