tengqm commented on code in PR #5801:
URL: https://github.com/apache/gravitino/pull/5801#discussion_r1881487352
##########
bundles/aws-bundle/src/main/java/org/apache/gravitino/s3/credential/S3TokenProvider.java:
##########
@@ -104,6 +104,10 @@ private StsClient createStsClient(S3CredentialConfig
s3CredentialConfig) {
if (StringUtils.isNotBlank(region)) {
builder.region(Region.of(region));
}
+ String stsEndpoint = s3CredentialConfig.stsEndpoint();
+ if (StringUtils.isNotBlank(stsEndpoint)) {
+ builder.endpointOverride(URI.create(stsEndpoint));
Review Comment:
I tend to agree. A thorough check can/should be done in a utils package.
So the conclusion is that we are leaning towards to a trial&error approach.
Let the `URI.create()` try and we are prepared to handle the exception.
Two things left then (as commented previously):
- Shall we check the blank string case? If `URI.create()` silently accepts
an empty
string and we hate it, we should add an additional check here. Preferably
adding a
comment `// URI.create sucks, it cannot detect empty string. This is a
stricter check`.
- How we are handling the exception? When something bad happens in this call,
we cannot just simply leave it there. There could be previous changes we
need to
roll back (e.g. line 105, assuming it has a persistent impact).
My personal favor is to kill and handle the exception wherever it raises.
Propagating the exception up the call chain has always been a nightmare to
me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
