Skip to content

Add option to force S3 region to connect to#14398

Merged
Praveen2112 merged 1 commit intotrinodb:masterfrom
skrzypo987:skrzypo/121-s3-choose-region
Oct 5, 2022
Merged

Add option to force S3 region to connect to#14398
Praveen2112 merged 1 commit intotrinodb:masterfrom
skrzypo987:skrzypo/121-s3-choose-region

Conversation

@skrzypo987
Copy link
Copy Markdown
Member

Description

Pretty straightforward change allowing to force S3 client to use specified region

Non-technical explanation

Add option to force specific region when connecting to S3

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Add option to force specific region when connecting to S3

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Sep 30, 2022

@findepi / @ebyhr Reminder to make sure Glue and cloud-only tests are run before merge.

@skrzypo987
Copy link
Copy Markdown
Member Author

@findepi / @ebyhr Reminder to make sure Glue and cloud-only tests are run before merge.

@hashhar Do I need to add some tags to enable them?

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Sep 30, 2022

a non-fork PR needs sending since it requires secrets. Unfortunately nothing you can do for it yet @skrzypo987.

Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Can we update hive.rst?

@findepi
Copy link
Copy Markdown
Member

findepi commented Oct 1, 2022

Test PR with secrets: #14419
(@nineinchnick's #12817 will make these PRs easier)

@@ -920,7 +921,7 @@ private AmazonS3 createAmazonS3Client(Configuration hadoopConfig, ClientConfigur

Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 Oct 4, 2022

Choose a reason for hiding this comment

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

You should use the region even if endpoint isn't set

        String endpoint = hadoopConfig.get(S3_ENDPOINT);
        String region = hadoopConfig.get(S3_REGION);
        if (endpoint != null) {
            clientBuilder.setEndpointConfiguration(new EndpointConfiguration(endpoint, region));
            regionOrEndpointSet = true;
        }
        else if (region != null) {
            clientBuilder.setRegion(region);
            regionOrEndpointSet = true;
        }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are actually right. I totally ignored the fact that setting regionOrEndpointSet to true will prevent US_EAST_1 to be set as a default few lines later.
It will not effectively change anything as US_EAST_1 is always chosen as a default one, but we should set it nonetheless.

Made the change and altered tests.

@skrzypo987 skrzypo987 force-pushed the skrzypo/121-s3-choose-region branch from ca58af8 to 2e605a3 Compare October 4, 2022 15:52
Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

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

👍

@Praveen2112 Praveen2112 merged commit fa24a13 into trinodb:master Oct 5, 2022
@github-actions github-actions bot added this to the 399 milestone Oct 5, 2022
@colebow
Copy link
Copy Markdown
Member

colebow commented Oct 5, 2022

If we're going to add a configuration property, we also need to document it. I'll submit a follow-up PR.

cc @Praveen2112

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

Development

Successfully merging this pull request may close these issues.

7 participants