Skip to content

Conversation

@munendrasn
Copy link
Contributor

@munendrasn munendrasn commented Oct 5, 2024

This PR is on top of the changes in PR #9804 from @ebelgasmi12 .
Additionally, The PR contains integration test for cross region changes.

cc @elmehdibelgasmi @singhpk234

Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me ! would request to mark original author of PR as co-author in the pr description

cc @flyrain @RussellSpitzer
you might also wanna take a look as because of this one had to create PolarisS3FileIOClientFactory

Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Wondering if the aws client can allow missing the client.region config now. cc @jackye1995

@munendrasn
Copy link
Contributor Author

Thank you @singhpk234 @flyrain for the review.
As suggested, I have updated the PR description to include the original PR and author. Please let me know if it looks good.

I would wait at least for couple of days for @ebelgasmi12 to respond. Maybe, we can cherry pick commits and add it to original PR, so that original PR can be merged

@munendrasn
Copy link
Contributor Author

@flyrain @singhpk234 @jackye1995 @RussellSpitzer
If the changes looks good, would it be possible merge the changes? It would great if this can make it to 1.7 release FYI @ebelgasmi12

@jackye1995 jackye1995 self-requested a review October 10, 2024 14:15
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@jackye1995
Copy link
Contributor

Wondering if the aws client can allow missing the client.region config now

@flyrain what do you mean by this? I think that config has been optional? And it will try to resolve the region using the default region chain https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/region-selection.html#automatically-determine-the-aws-region-from-the-environment

@flyrain
Copy link
Contributor

flyrain commented Oct 14, 2024

@jackye1995, I meant the AwsClientProperties client.region. I believe it is still mandatory, no? To be clear, it is not related to this PR. I was wondering it since s3 basically can allow client to not set region now.

@flyrain flyrain merged commit 3d9fc1d into apache:main Oct 14, 2024
@flyrain
Copy link
Contributor

flyrain commented Oct 14, 2024

Thanks @munendrasn for the PR. Thanks all for the review.

@jackye1995
Copy link
Contributor

I meant the AwsClientProperties client.region. I believe it is still mandatory, no?

No I don't think it is mandatory, I never set this in my sessions at least. If you find it otherwise, we should probably fix it.

@munendrasn
Copy link
Contributor Author

Thanks @ebelgasmi12 for the original changes, Thank you all for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants