Skip to content

Adjust connection pool parameters for S3 based exchange#11996

Merged
arhimondr merged 2 commits intotrinodb:masterfrom
arhimondr:exchange-adjust-connection-pool
Apr 19, 2022
Merged

Adjust connection pool parameters for S3 based exchange#11996
arhimondr merged 2 commits intotrinodb:masterfrom
arhimondr:exchange-adjust-connection-pool

Conversation

@arhimondr
Copy link
Contributor

Description

Reduce maximum number of concurrent requests and increase timeout for obtaining a connection from a pool.

Generally only ~15 connections should be enough to utilize 10Gbit / s NIC in one direction. 100 connections should be more than necessary. Allowing to many connections is likely to create unnecessary contention.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Exchange

How would you describe this change to a non-technical end user or system administrator?

-

Related issues, pull requests, and links

-

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@arhimondr arhimondr requested review from linzebing and losipiuk April 18, 2022 17:16
@cla-bot cla-bot bot added the cla-signed label Apr 18, 2022
@losipiuk
Copy link
Member

CI: #11876

@arhimondr arhimondr merged commit 8e34a15 into trinodb:master Apr 19, 2022
@arhimondr arhimondr deleted the exchange-adjust-connection-pool branch April 19, 2022 15:01
@github-actions github-actions bot added this to the 378 milestone Apr 19, 2022
return asyncClientMaxPendingConnectionAcquires;
}

@Config("exchange.s3.async-client-max-pending-connection-acquires")
Copy link
Member

Choose a reason for hiding this comment

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

@arhimondr @losipiuk .. did we not want to document this property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pretty low level and it should generally not be changed. Not sure if it makes sense to document.

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.

4 participants