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

Fix incorrect ThreadPool usage after ThreadPool introspection #48244

Merged
merged 3 commits into from
Mar 30, 2023

Conversation

azat
Copy link
Collaborator

@azat azat commented Mar 30, 2023

$ gg 'ThreadPool[^()]([A-Za-z_]\+,' src/
src/Interpreters/Context.cpp:        shared->load_marks_threadpool = std::make_unique<ThreadPool>(pool_size, pool_size, queue_size);
src/Interpreters/Context.cpp:        shared->prefetch_threadpool = std::make_unique<ThreadPool>(pool_size, pool_size, queue_size);
src/Interpreters/Context.cpp:        shared->threadpool_writer = std::make_unique<ThreadPool>(pool_size, pool_size, queue_size);

Fixes: #47880
Closes: #48234

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

```
$ gg 'ThreadPool[^()]([A-Za-z_]\+,' src/
src/Interpreters/Context.cpp:        shared->load_marks_threadpool = std::make_unique<ThreadPool>(pool_size, pool_size, queue_size);
src/Interpreters/Context.cpp:        shared->prefetch_threadpool = std::make_unique<ThreadPool>(pool_size, pool_size, queue_size);
src/Interpreters/Context.cpp:        shared->threadpool_writer = std::make_unique<ThreadPool>(pool_size, pool_size, queue_size);
```

Fixes: ClickHouse#47880
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 30, 2023
@tavplubix
Copy link
Member

Do we have other incorrect usages? Let's make the compiler find them (it's okay to to in a separate PR)

@tavplubix tavplubix self-assigned this Mar 30, 2023
@tavplubix tavplubix added the skip mergeable check The label to omit checks for required statuses. Does not help with `pr-feature` new docs check label Mar 30, 2023
@tavplubix
Copy link
Member

Style check is broken in master, so it's impossible to merge anything...

@tavplubix tavplubix merged commit 46e8535 into ClickHouse:master Mar 30, 2023
@azat
Copy link
Collaborator Author

azat commented Mar 31, 2023

Do we have other incorrect usages? Let's make the compiler find them (it's okay to to in a separate PR)

The problem is that before #47880 there was the following ctor:

explicit ThreadPoolImpl(size_t max_threads_); /* 1 */

ThreadPoolImpl(
    size_t max_threads_,
    size_t max_free_threads_,
    size_t queue_size_,
    bool shutdown_on_exception_ = true); /* 2 */

After:

explicit ThreadPoolImpl(
    Metric metric_threads_,
    Metric metric_active_threads_,
    size_t max_threads_); /* 1 */

ThreadPoolImpl(
    Metric metric_threads_,
    Metric metric_active_threads_,
    size_t max_threads_,
    size_t max_free_threads_,
    size_t queue_size_,
    bool shutdown_on_exception_ = true); /* 2 */

And Metric is size_t, so compiler does not show any error when max_threads, max_free_threads, queue_size was passed as metric_threads, metric_active_threads, max_threads and ctor 1 was used instead of 2

@azat
Copy link
Collaborator Author

azat commented Mar 31, 2023

@tavplubix
Copy link
Member

And Metric is size_t, so compiler does not show any error when max_threads, max_free_threads, queue_size was passed as metric_threads, metric_active_threads, max_threads and ctor 1 was used instead of 2

Yes, I understand the problem. I mean that we can change ctors somehow, so it will be impossible to pass invalid arguments. It's great when the compiler can validate code. For example, we can create a structure like ThreadPoolSettings or try using StrongTypedef or something else.

@azat azat deleted the fix-threadpools branch April 1, 2023 09:14
@azat
Copy link
Collaborator Author

azat commented Apr 1, 2023

@tavplubix yeah, I thought about this, but this will require recompile lots of stuff, so decided to postpone this, anyway here is a PR, that uncovers another, last, bug - #48314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-not-for-changelog This PR should not be mentioned in the changelog skip mergeable check The label to omit checks for required statuses. Does not help with `pr-feature` new docs check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants