Skip to content
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

Consistent AWS timeouts #22594

Merged

Conversation

excitoon
Copy link
Contributor

@excitoon excitoon commented Apr 4, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Bug Fix

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

Fixed a bug with unlimited wait for auxiliary AWS requests.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Apr 4, 2021
@excitoon
Copy link
Contributor Author

excitoon commented Apr 4, 2021

Related: #21852

@nikitamikhaylov nikitamikhaylov self-assigned this Apr 5, 2021
@excitoon
Copy link
Contributor Author

excitoon commented Apr 5, 2021

Failed tests are unrelated.

Copy link
Member

@nikitamikhaylov nikitamikhaylov left a comment

Choose a reason for hiding this comment

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

BTW, where was the bug? And why these changes fix it?

@excitoon
Copy link
Contributor Author

excitoon commented Apr 15, 2021

@nikitamikhaylov the bug was in few places (not sure if I found every one), for example in S3CredentialsProviderChain:

                /// EC2MetadataService throttles by delaying the response so the service client should set a large read timeout.
                /// EC2MetadataService delay is in order of seconds so it only make sense to retry after a couple of seconds.
                aws_client_configuration.connectTimeoutMs = 1000;
                aws_client_configuration.requestTimeoutMs = 1000;
                aws_client_configuration.retryStrategy = std::make_shared<Aws::Client::DefaultRetryStrategy>(1, 1000);

                auto ec2_metadata_client = std::make_shared<Aws::Internal::EC2MetadataClient>(aws_client_configuration);
                auto config_loader = std::make_shared<Aws::Config::EC2InstanceProfileConfigLoader>(ec2_metadata_client);

                AddProvider(std::make_shared<Aws::Auth::InstanceProfileCredentialsProvider>(config_loader));
                LOG_INFO(logger, "Added EC2 metadata service credentials provider to the provider chain.");

It uses "default" parameters to set timeouts but in our code we switched to another properties at some moment. Actually, requestTimeoutMs is more appropriate name for this, because it effectively sets timeout on a socket, for single read, and this is not about HTTP at all.

Also, in Aws::Internal::MakeDefaultHttpResourceClientConfiguration() (this code shall not be used right now):

            // EC2MetadataService throttles by delaying the response so the service client should set a large read timeout.
            // EC2MetadataService delay is in order of seconds so it only make sense to retry after a couple of seconds.
            res.connectTimeoutMs = 1000;
            res.requestTimeoutMs = 1000;
            res.retryStrategy = Aws::MakeShared<DefaultRetryStrategy>(RESOURCE_CLIENT_CONFIGURATION_ALLOCATION_TAG, 1, 1000);

            return res;

@nikitamikhaylov nikitamikhaylov merged commit a6b43f6 into ClickHouse:master Apr 15, 2021
kitaisreal added a commit that referenced this pull request Apr 16, 2021
Backport #22594 to 21.2: Consistent AWS timeouts
kitaisreal added a commit that referenced this pull request Apr 16, 2021
Backport #22594 to 21.3: Consistent AWS timeouts
kitaisreal added a commit that referenced this pull request Apr 16, 2021
Backport #22594 to 21.4: Consistent AWS timeouts
alexey-milovidov added a commit that referenced this pull request Apr 17, 2021
Backport #22594 to 20.8: Consistent AWS timeouts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants