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

[Native] Make PrestoServer create and pass connectorCpuExecutor to Connector #24259

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

gggrace14
Copy link
Contributor

@gggrace14 gggrace14 commented Dec 13, 2024

Description

Create a CPUThreadPoolExecutor data member, connectorCpuExecutor_, for PrestoServer.
Pass it to every created Connector. Add a new config connector.num-cpu-threads-hw-multiplier
to control how many threads would be used for the executor.
connector.num-cpu-threads-hw-multiplier will set connectorCpuExecutor_ to nullptr.

Motivation and Context

Make a process-wise managed CPUThreadPoolExecutor instance available to all connectors.
Connector could schedule CPU-bound async operators to it so that they will not occupy
the driver thread pool.

Impact

When the new config connector.num-cpu-threads-hw-multiplier is set to positive,
a process-wise CPUThreadPoolExecutor will be created.

Test Plan

Release Notes

== NO RELEASE NOTE ==

@gggrace14 gggrace14 requested review from Yuhta and xiaoxmeng December 13, 2024 19:31
@gggrace14 gggrace14 requested a review from a team as a code owner December 13, 2024 19:31
Comment on lines 1175 to 1187
const auto numConnectorCpuThreads = std::max<size_t>(
SystemConfig::instance()->connectorNumCpuThreadsHwMultiplier() *
std::thread::hardware_concurrency(),
0);
Copy link
Contributor Author

@gggrace14 gggrace14 Dec 13, 2024

Choose a reason for hiding this comment

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

Can you guys help confirm if we still need this multiplier mechanism to derive # of cpu threads?

I will double check and handle the config names later. It's being referenced and set to 0 in prod.

@gggrace14 gggrace14 marked this pull request as draft December 13, 2024 20:15
@gggrace14 gggrace14 changed the title [Native] Change to create and pass CPUThreadPoolExecutor to Connector… [Native] Make PrestoServer create and pass connectorCpuExecutor to Connector Dec 14, 2024
xiaoxmeng
xiaoxmeng previously approved these changes Dec 14, 2024
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@gggrace14 LGTM. Thanks!

@@ -1172,6 +1180,20 @@ std::vector<std::string> PrestoServer::registerConnectors(
const fs::path& configDirectoryPath) {
static const std::string kPropertiesExtension = ".properties";

const auto numConnectorCpuThreads = std::max<size_t>(
Copy link
Contributor

Choose a reason for hiding this comment

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

you might want to track the connector cpu executor stats in followup. Thanks!

Copy link
Contributor Author

@gggrace14 gggrace14 Dec 15, 2024

Choose a reason for hiding this comment

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

Will check with you about what stats will be useful to expose offline

xiaoxmeng
xiaoxmeng previously approved these changes Dec 14, 2024
…nnector

Create a CPUThreadPoolExecutor data member, connectorCpuExecutor_, for PrestoServer.
Pass it to every created Connector. Add a new config `connector.num-cpu-threads-hw-multiplier`
to control how many threads would be used for the executor.
`connector.num-cpu-threads-hw-multiplier` will set connectorCpuExecutor_ to nullptr.

Make a process-wise managed CPUThreadPoolExecutor instance available to all connectors.
Connector could schedule CPU-bound async operators to it so that they will not occupy
the driver thread pool.
@gggrace14 gggrace14 marked this pull request as ready for review December 15, 2024 04:32
@gggrace14 gggrace14 requested a review from xiaoxmeng December 15, 2024 19:07
@gggrace14
Copy link
Contributor Author

The test failure is "Execution of 'actual' query failed" as opposed to different results. Looks like not related to this change but a framework issue.

The failed tests are
TestPrestoSparkNativeGeneralQueries>AbstractTestNativeGeneralQueries.testIsNullIsNotNull
TestPrestoSparkNativeWindowQueries>AbstractTestNativeWindowQueries.testFirstValueOrderKey

@xiaoxmeng xiaoxmeng merged commit 2183c0a into prestodb:master Dec 16, 2024
67 of 68 checks passed
@gggrace14 gggrace14 deleted the cpuexec branch December 16, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants