Define S3 client with bucket and endpoint resolution by antonio2368 · Pull Request #45783 · ClickHouse/ClickHouse

antonio2368 · 2023-01-30T13:36:34Z

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Improve internal S3 client to correctly deduce regions and redirections for different types of URLs.

I defined a client that will try to detect region for the endpoint (if it's not explicitly defined in the URL or config) by using HeadBucket allowing us to use HeadObject correctly.
Regions will be cached for each bucket per client.

Additionally, I noticed that the SDK doesn't handle correctly code 301 when the style of URL changes.
For example, this never worked with CH:

SELECT * FROM s3('https://s3.amazonaws.com/clickhouse-public-datasets/wikistat/partitioned/wikistat201801.native.zst') limit 10

because redirect is to a different style URL (virtual address) while AWS SDK simply changes URI authority with the location in the body.
I defined a similar logic with cache as for the region and now this also correctly works, even with HeadObject.

To verify that both caches are working, you can define an S3 table and check the performance difference between first and second run (if an URL without region is used and even more if it's redirected).

Things left to do:

go through the code and try to include it in other places (e.g. BackupS3)
see if tests are happy with the changes
add SYSTEM DROP S3 CLIENT CACHE

AWS SDK PR ClickHouse/aws-sdk-cpp#13

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

src/IO/S3/copyS3File.h

vitlibar · 2023-02-03T13:28:36Z

src/IO/S3/Requests.h

+    {
+        auto params = BaseRequest::GetEndpointContextParams();
+        if (!region_override.empty())
+            params.emplace_back("Region", region_override);


Is it possible that BaseRequest::GetEndpointContextParams() already has this override?

Could be possible but the params are processed sequentially where the latest will always override the previous.
Also, the params are fetched from 3 different sources where BaseRequest::GetEndpointContextParams is the last one so we are sure it will always win. An assumption we need to be careful of if they change it in SDK (don't see a reason to do it but it could happen). I need to add a comment explaining exactly this.

vitlibar · 2023-02-03T13:36:33Z

src/IO/S3/Client.h

+
+    ClientCache(const ClientCache & other)
+        : region_for_bucket_cache(other.region_for_bucket_cache)
+        , uri_for_bucket_cache(other.uri_for_bucket_cache)


Do we need to copy this cache?

No, but I didn't see a reason not to do it. We copy the client for globbed URLs and if it's copied than the endpoint is for sure the same so why not reuse the cache.

vitlibar · 2023-02-03T13:38:05Z

src/IO/S3/Client.h

+    ClientCacheRegistry() = default;
+
+    std::mutex clients_mutex;
+    std::unordered_map<ClientCache *, std::weak_ptr<ClientCache>> client_caches;


Is it necessary to have this two-dimensional system? I mean can't we just keep region_for_bucket_cache and uri_for_bucket_cache here in this singleton class?

We can but I wanted a way to detect if a client is deleted but the destructor failed to unregister it so we can clean the dead ptrs here if that happens.
If it was premature optimization I can simplify in the way as you described.

vitlibar · 2023-02-03T13:40:16Z

src/IO/S3/Requests.h

+    mutable std::optional<S3::URI> uri_override;
+};
+
+using HeadObjectRequest = ExtendedRequest<Model::HeadObjectRequest>;


Can't we just modify BaseRequest without inheriting every S3 request type?

I wanted to avoid modifying the SDK because it's more complex to extend with new types (update fork then update the commit of the submodule). This way it's much easier to add support for new type of request IMO.

src/IO/S3/Client.h

vitlibar · 2023-02-03T13:44:25Z

src/IO/S3/Client.h

+        ClientCacheRegistry::instance().registerClient(cache);
+    }
+
+    /// Make regular functions private


If you use requests from SDK the client can't override the region/endpoint so we make the function accepting Aws::S3::Model::*Request as private and the only way to call the functions with our client is with our extended requests.

src/IO/S3/getObjectInfo.cpp

src/IO/S3/Client.h

vitlibar · 2023-02-03T17:55:46Z

src/IO/S3/Client.h

+        , max_redirects(max_redirects_)
+        , log(&Poco::Logger::get("S3Client"))
+    {
+        auto * endpoint_provider = dynamic_cast<Aws::S3::Endpoint::S3DefaultEpProviderBase *>(accessEndpointProvider().get());


Can endpoint_provider be nullptr?

it shouldn't be, AWS always does a nullptr check before accessing it, I can do chassert.

vitlibar · 2023-02-06T12:01:24Z

src/IO/S3/Client.h

+
+    template <typename RequestType, typename RequestFn>
+    std::invoke_result_t<RequestFn, RequestType>
+    doRequest(const RequestType & request, RequestFn request_fn) const


This template function is not called anywhere except Client.cpp so it can be easily moved to Client.cpp.

vitlibar · 2023-02-06T12:01:57Z

src/IO/S3/Client.h

+        detect_region = explicit_region == Aws::Region::AWS_GLOBAL && endpoint.find(".amazonaws.com") != std::string::npos;
+
+        cache = std::make_shared<ClientCache>();
+        ClientCacheRegistry::instance().registerClient(cache);


Let's move the constructor to Client.cpp

vitlibar · 2023-02-06T12:15:12Z

src/IO/S3/Client.cpp

+    if (auto region = getRegionForBucket(bucket); !region.empty())
+    {
+        if (!detect_region)
+            LOG_INFO(log, "Using region override {} for bucket {}", region, bucket);


If the endpoint is specified with an explicit region then detect == false and we will always see this line in the logs. Looks excessive.

Only if we explicitly defined wrong region, if it was correct no log will be printed.
But yeah, it can still be excessive.

vitlibar · 2023-02-06T12:21:11Z

src/IO/S3/Client.cpp

+    if (checkIfWrongRegionDefined(bucket, error, new_region))
+    {
+        request.overrideRegion(new_region);
+        return HeadObject(request);


The HeadObject function doesn't have to be recursive here. If possible let's use just a cycle, usually they're easier for understanding and debugging.

vitlibar · 2023-02-06T12:34:49Z

src/IO/S3/Client.h

+            {
+                auto uri_override = request.getURIOverride();
+                assert(uri_override.has_value());
+                updateURIForBucket(bucket, std::move(*uri_override));


There is only one place where you set found_new_endpoint later - let's move this code there. SCOPE_EXIT makes the code harder to read.

vitlibar · 2023-02-06T12:40:46Z

src/IO/S3/Client.cpp

+        request.overrideRegion(std::move(region));
+    }
+
+    if (auto uri = getURIForBucket(bucket); uri.has_value())


I think we can add the function empty() to S3::URI and so make the code more consistent without using std::optional<S3::URI>.

vitlibar · 2023-02-06T12:57:37Z

src/IO/S3/Client.cpp

+    return wrapped_strategy->RequestBookkeeping(httpResponseOutcome, lastError);
+}
+
+bool Client::checkIfWrongRegionDefined(const std::string & bucket, const Aws::S3::S3Error & error, std::string & region) const


Let's rename the function to checkIfExplicitRegionIsWrong for consistency with the variable explicit_region.

vitlibar · 2023-02-06T13:20:53Z

src/IO/S3/Client.h

+            }
+        );
+
+        for (size_t attempt = 0; attempt <= max_redirects; ++attempt)


request_fn already has its own retries, see AWSClient::AttemptExhaustively(). And our code which uses S3::Client often has its own retries too. I'm afraid there can be too many retries.

We already do something similar with PocoHTTPClient and with s3_max_redirects.
This is basically a redirect attempt + it should be much rarer with 1 or 2 jumps.

vitlibar · 2023-02-06T15:08:48Z

src/IO/S3/Client.cpp

+        region = GetErrorMarshaller()->ExtractRegion(error);
+
+        if (region.empty())
+            region = getRegionForBucket(bucket, /*force_detect*/ true);


I don't understand why we need to call HeadBucket after our request failed. In all known cases if a request fails because of the region the correct region would be in the response body. With one exception - HeadObject but we cover this case with HeadBucket before the main request.

if the region is explicitly defined, the HeadBucket is not called.

vitlibar · 2023-02-06T15:12:38Z

src/IO/S3/Client.cpp

+    auto bucket_uri = getURIForBucket(bucket);
+    if (!bucket_uri)
+    {
+        if (auto maybe_error = updateURIForBucketForHead(bucket); maybe_error.has_value())


I don't think we should make our code that complicated. If HeadBucket and the HeadObject's response header both didn't help us to get a correct region then ListObjects will hardly help.

It will help us if we got 301 but we couldn't get the location because HEAD doesn't have a response body.

vitlibar · 2023-02-06T15:17:14Z

I thought about this PR a while.

I hope that time when I updated AWS SDK in out contrib wasn't the last time we did it. And this PR introduced many changes which will make that updating quite hard. Inheriting AWS::S3::S3Client was ok, but inheriting all requests along copy-pasting a lot of changed code from AWS retry system don't feel right.

It seems all we needed were:

Store regionFromResponse and newUri somewhere.
Reuse region and uri from 1 for other requests later.
Before sending HeadObject request we need to send HeadBucket first if it's an amazon endpoint and there isn't an explicit region in here.

For 1 we could add two virtual functions to AWSClient:

virtual void GotRegionFromResponse(const Aws::Http::URI & uri, const std::string & regionFromResponse) {}
virtual void GotURIFromResponse(const Aws::Http::URI & uri, const std::string & uriFromResponse) {}

then call them from AWSClient::AttemptExhaustively and override them in our S3::Client.

For 2 we could add two virtual functions to AWSClient:

virtual void MaybeOverrideURI(Aws::Http::URI & uri) {}
virtual void MaybeOverrideSigningRegion(std::string & signingRegion, const Aws::Http::URI & uri) {}

and again call them from AWSClient::AttemptExhaustively and override them in our S3::Client.

And for 3 we could call HeadBucket from our S3::Client::HeadObject once (without retries) and then proceed to normal AWSClient::HeadObject. And let AWS SDK do retries as it wants.

What's the difference? While updating AWS SDK it's much easier to resolve a few small conflicts because of we added a few empty functions than trying to understand what happened with that very complicated retry mechanism in our code or trying to figure out what we should do with new requests possibly added by AWS in the future - should we inherit them too or not?

antonio2368 · 2023-02-06T16:33:25Z

I can try doing it in this way, but I think we should do HeadBucket every time the region is not defined because for other requests there are no guarantees that the correct region will be returned, even for 301.

S3::Client::HeadObject is still a problem because URI redirects are in response body, something HEAD response doesn't have so that's why I did ListObject in the current implementation.
Trying to fit that into AttemptExhaustively could be tricky.

vitlibar · 2023-02-08T17:12:15Z

Do you know any example where some non-HeadObject request failed because of undefined region but the response didn't contain the correct region?

antonio2368 · 2023-02-13T11:23:44Z

Not sure now, I think I did see a request like that but why not cover this case just in case?
I was mostly using the Java client as a reference where they fetch bucket location on every request regardless of the defined region.

Alima777 · 2023-09-19T13:45:55Z

Hi, @antonio2368. Is there any benchmark on this improved client?

Just like @vitlibar said, it's a great modification which may bring trouble when updating aws submodules. So I'd like to know how much improvement this client can bring?

antonio2368 · 2023-09-20T07:50:33Z

Hi @Alima777
there was no benchmark just for the client but the point of this PR was to make client more robust to different endpoint redirects and better region deduction.
When it comes to the performance, it was only important not to make it worse.

But there are many other client improvements already introduced and planned to be introduced in other PRs.

antonio2368 added 2 commits January 30, 2023 13:28

Update aws

1e197ef

Define S3 client with bucket and endpoint resolution

7addc3c

robot-ch-test-poll4 added pr-improvement Pull request with some product improvements submodule changed At least one submodule changed in this PR. labels Jan 30, 2023

Add defines for ErrorCodes

113ca78

antonio2368 force-pushed the s3-with-head-bucket branch from 460f9f2 to 113ca78 Compare January 30, 2023 14:50

antonio2368 added 13 commits January 31, 2023 10:04

Use S3Client everywhere

f24b71e

Merge branch 'master' into s3-with-head-bucket

8fa070a

Remove unused errorcode

aa30444

Add DROP S3 CLIENT CACHE query

8009658

Add a comment

df24312

Fix style

b7f7a42

Update aws

2253ae9

Update reference files

b0da5bf

Add missing include

65d36d4

Merge branch 'master' into s3-with-head-bucket

8e155bc

Fix unit test

70c6de2

Remove unneeded declarations

553d1c3

Merge branch 'master' into s3-with-head-bucket

4268be6

vitlibar reviewed Feb 1, 2023

View reviewed changes

src/IO/S3/copyS3File.h Outdated Show resolved Hide resolved

vitlibar self-assigned this Feb 1, 2023

antonio2368 added 3 commits February 2, 2023 09:23

Correctly use RetryStrategy

00d1e77

Rename S3Client to Client

3c5f1bd

Merge branch 'master' into s3-with-head-bucket

7d3c16c

antonio2368 marked this pull request as ready for review February 2, 2023 10:15

antonio2368 and others added 4 commits February 2, 2023 13:52

Fix retry count

59058a7

Merge branch 'master' into s3-with-head-bucket

eef0211

fix clang-tidy warnings

157392c

Merge branch 'master' into s3-with-head-bucket

eb10b50

nikitamikhaylov approved these changes Feb 3, 2023

View reviewed changes

nikitamikhaylov merged commit d5117f2 into master Feb 3, 2023

nikitamikhaylov deleted the s3-with-head-bucket branch February 3, 2023 13:30

vitlibar reviewed Feb 3, 2023

View reviewed changes

src/IO/S3/Client.h Show resolved Hide resolved

vitlibar reviewed Feb 3, 2023

View reviewed changes

src/IO/S3/getObjectInfo.cpp Show resolved Hide resolved

vitlibar reviewed Feb 3, 2023

View reviewed changes

src/IO/S3/Client.h Show resolved Hide resolved

vitlibar reviewed Feb 3, 2023

View reviewed changes

antonio2368 mentioned this pull request Feb 6, 2023

Polish S3 client #46070

Merged

1 task

vitlibar reviewed Feb 6, 2023

View reviewed changes

ianton-ru mentioned this pull request Jun 24, 2025

Antalya 25.3: Support different warehouses behind Iceberg REST catalog Altinity/ClickHouse#860

Merged

13 tasks

Conversation

antonio2368 commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonio2368 Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonio2368 Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonio2368 commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitlibar commented Feb 8, 2023

Uh oh!

antonio2368 commented Feb 13, 2023

Uh oh!

Alima777 commented Sep 19, 2023

antonio2368 commented Jan 30, 2023 •

edited

Loading

vitlibar Feb 3, 2023 •

edited

Loading

antonio2368 Feb 3, 2023 •

edited

Loading

antonio2368 Feb 7, 2023 •

edited

Loading

vitlibar commented Feb 6, 2023 •

edited

Loading

antonio2368 commented Feb 6, 2023 •

edited

Loading