Skip to content

Make settings controlling connection drop on overloaded CPU hot-reloadable#79052

Merged
Algunenano merged 7 commits intomasterfrom
alex-dc
May 21, 2025
Merged

Make settings controlling connection drop on overloaded CPU hot-reloadable#79052
Algunenano merged 7 commits intomasterfrom
alex-dc

Conversation

@alexkats
Copy link
Copy Markdown
Contributor

@alexkats alexkats commented Apr 11, 2025

Changelog category (leave one):

  • Improvement

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

Make settings controlling connection drop on overloaded CPU hot-reloadable

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Apr 11, 2025

Workflow [PR], commit [0618c8f]

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 11, 2025
@alexkats alexkats closed this Apr 11, 2025
@alexkats alexkats reopened this Apr 11, 2025
@alexkats alexkats force-pushed the alex-dc branch 3 times, most recently from 08ed875 to fc1d26f Compare April 15, 2025 14:50
@serxa serxa self-assigned this Apr 22, 2025
/// Only for system.server_settings, actually value stored in reloader itself
std::atomic_size_t config_reload_interval_ms = ConfigReloader::DEFAULT_RELOAD_INTERVAL.count();

double min_os_cpu_wait_time_ratio_to_drop_connection = 15.0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't use the same cakes as for the settings?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understood it right, but if you mean why not use the same values as from the Settings.cpp, then the answer is those are different settings. min_os_cpu_wait_time_ratio_to_drop_connection controls connection drops, while min_os_cpu_wait_time_ratio_to_throw controls query rejection, and those should have different values

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, right I missed it

@serxa
Copy link
Copy Markdown
Member

serxa commented May 14, 2025

@Algunenano, I see you have assigned to this PR. Should we merge it? It seems okay to me to merge it, but I cannot understand what the problem is with the docs check.

@Algunenano
Copy link
Copy Markdown
Member

We need #80211 first because this feature can't be enabled by default. Then I'll adjust the PR

@Algunenano Algunenano added pr-improvement Pull request with some product improvements and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels May 19, 2025
@Algunenano
Copy link
Copy Markdown
Member

@Algunenano
Copy link
Copy Markdown
Member

@Algunenano Algunenano enabled auto-merge May 21, 2025 11:59
@Algunenano Algunenano added this pull request to the merge queue May 21, 2025
Merged via the queue into master with commit e774b60 May 21, 2025
117 of 122 checks passed
@Algunenano Algunenano deleted the alex-dc branch May 21, 2025 12:19
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants