iframe-proxy

al13n321 · 2026-03-26T02:49:25Z

Changelog category (leave one):

Improvement

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

A few Keeper server settings are now hot-reloaded if config file is changed at runtime: max_requests_batch_size, max_requests_batch_bytes_size, max_request_size, quorum_reads.

I got tired of restarting the server all the time when benchmarking, so here's a way to tweak things on the fly.

To make a setting hot-reloadable, add HOT_RELOAD flag to its declaration in CoordinationSettings.cpp, and use keeper_context->getCoordinationSettings() to get the latest value.

This turned out much easier than expected. I always thought this would have to be slow or complicated (rcu! hazard pointers! thread-cached refcount!) and require changing all the call sites. But no, it's fine, you can just cache a shared_ptr in a thread-local variable to avoid all cacheline contention.

This PR only makes a couple easiest settings reloadable, just to make sure it works: max_requests_batch_size, max_requests_batch_bytes_size, quorum_reads.

Maybe we should do this to ServerSettings too, maybe replacing some of the laborious manual change propagation in the config reload callback.

Version info

Merged into: 26.4.1.826

…ings

clickhouse-gh · 2026-03-26T02:50:24Z

Workflow [PR], commit [341039b]

Summary: ❌
Keeper Stress Tests (PR): Grafana: Run details (this run), Grafana: 1vs1 Comparison, Grafana: Historical progression

job_name	test_name	status	info
Stateless tests (amd_asan_ubsan, flaky check)		failure
	00084_external_aggregation	FAIL	cidb
	00084_external_aggregation	FAIL	cidb
Stateless tests (amd_tsan, flaky check)		failure
	00084_external_aggregation	FAIL	cidb
Stateless tests (amd_debug, flaky check)		failure
	00084_external_aggregation	FAIL	cidb
Stateless tests (amd_binary, flaky check)		failure
	00084_external_aggregation	FAIL	cidb
	00084_external_aggregation	FAIL	cidb
	00084_external_aggregation	FAIL	cidb
	00084_external_aggregation	FAIL	cidb
	00084_external_aggregation	FAIL	cidb
Stress test (arm_msan)		failure
	Server died	FAIL	cidb, issue
	MemorySanitizer: use-of-uninitialized-value (STID: 2727-3cba)	FAIL	cidb

AI Review

Summary

This PR introduces hot-reload support for selected Keeper coordination settings (max_requests_batch_size, max_requests_batch_bytes_size, max_request_size, quorum_reads) by splitting fixed vs dynamic settings access and wiring dynamic reads into request-processing paths. The direction is good, but there are still correctness-of-tests gaps and one test assertion that is too generic and can mask regressions; verdict is to request changes before merge.

Findings

❌ Blockers
- [tests/integration/test_keeper_dynamic_settings/test.py:103] The test validates max_request_size behavior only through system.zookeeper (TCP Keeper path). This PR also changes HTTP storage handling (/api/v1/storage) to use dynamic max_request_size, but there is no coverage for HTTP, so regressions there can ship unnoticed.
- Suggested fix: add an integration test that performs HTTP storage writes before/after config reload and verifies large payloads are rejected only after reloading the limit.
⚠️ Majors
- [tests/integration/test_keeper_dynamic_settings/test.py:65] max_requests_batch_bytes_size was made HOT_RELOAD and is consumed in KeeperDispatcher::requestThread, but there is no behavior test covering this setting.
- Suggested fix: add a focused test that reloads max_requests_batch_bytes_size to a small value and checks that batching behavior changes accordingly (e.g., fewer large coalesced batches).
💡 Nits
- [tests/integration/test_keeper_dynamic_settings/test.py:122] pytest.raises(..., match="Connection loss") is too broad and can pass on unrelated disconnects, so it does not reliably prove max_request_size enforcement.
- Suggested fix: assert a limit-specific signal (LIMIT_EXCEEDED / Request size ... is too big request) or equivalent deterministic server-side evidence.

Tests

⚠️ Add HTTP-path coverage for hot-reloaded max_request_size (storage API path).
⚠️ Add behavior coverage for hot-reloaded max_requests_batch_bytes_size.

ClickHouse Rules

Item	Status	Notes
Deletion logging	➖
Serialization versioning	➖
Core-area scrutiny	✅
No test removal	✅
Experimental gate	➖
No magic constants	✅
Backward compatibility	✅
`SettingsChangesHistory.cpp`	➖
PR metadata quality	✅
Safe rollout	⚠️	Dynamic HTTP path lacks dedicated regression test coverage for hot-reloaded request-size behavior.
Compilation time	✅
No large/binary files	✅

Final Verdict

Status: ⚠️ Request changes
Minimum required actions:
- Add integration coverage for hot-reloaded max_request_size through Keeper HTTP storage API.
- Add integration coverage for hot-reloaded max_requests_batch_bytes_size behavior.
- Tighten max_request_size rejection assertion to a specific error signal instead of generic Connection loss.

antonio2368 · 2026-03-31T07:29:47Z

-        uint64_t max_batch_bytes_size = coordination_settings[CoordinationSetting::max_requests_batch_bytes_size];
-        size_t max_batch_size = coordination_settings[CoordinationSetting::max_requests_batch_size];
+        const auto & dynamic_settings = keeper_context->getDynamicSettings();
+        uint64_t operation_timeout_ms = dynamic_settings[CoordinationSetting::operation_timeout_ms].totalMilliseconds();


but you didn't mark it as hot reloadable

operation_timeout_ms is passed into nuraft, so wouldn't be easy to hot reload.

It's ok to use getDynamicSettings() for non-reloadable settings. Unless you think it would be better to have a style convention to always do getCoordinationSettings() even if there's already a perfectly good instance of CoordinationSettings right there in a local variable. (I was thinking the opposite: use getDynamicSettings everywhere and remove getCoordinationSettings. But that would require checking all call sites to fix the ones that hold on to the returned reference for too long.)

Ah now I see what is going on. It is a bit confusing and people could easily mess up as you have access to all settings in both.

So what do you propose? Convention to use getCoordinationSettings for non-reloadable settings? Split settings into two separate structs? Remove getCoordinationSettings and update all call sites to use getDynamicSettings? Merge as is? Abandon this PR? I'm ok with most of these options, unclear which is best.

I would just have getCoordinationSettings, and have the logic for dynamic settings.
Using getDynamicSettings everywhere could be misleading and understood as all settings here are dynamic.

Do you mean remove getDynamicSettings or have a convention to not use it for non-reloadable settings?

I meant removing getDynamic

Renamed getCoordinationSettings to getFixedCoordinationSettings, and getDynamicSettings to getCoordinationSettings.

alexey-milovidov · 2026-04-07T00:27:12Z

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

alexey-milovidov · 2026-04-08T02:22:10Z

The test 02859_replicated_db_name_zookeeper is fixed in #101952

…Settings -> getFixedSettings

alexey-milovidov · 2026-04-08T21:48:59Z

The 02346_text_index_bug101913 test failure is fixed by #102108 (already merged). Please rebase or merge master to pick up the fix.

antonio2368 · 2026-04-09T11:25:24Z

Update changelog, this is a very good change.

antonio2368 · 2026-04-10T09:20:00Z

Stress test (arm_msan) - tracking issue: #102241, should be fixed by #102305

clickhouse-gh · 2026-04-10T23:00:42Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.00%	84.20%	+0.20%
Functions	90.90%	90.90%	+0.00%
Branches	76.50%	76.70%	+0.20%

Changed lines: 83.48% (283/339) | lost baseline coverage: 8 line(s) · Uncovered code

Full report · Diff report

al13n321 · 2026-04-11T01:38:36Z

al13n321 added 3 commits March 25, 2026 19:45

Remove duplicate CoordinationSettings from KeeperConfigurationAndSett…

7f71ef6

…ings

Support hot-reloading keeper server settings

5250fb7

Test

b514957

clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 26, 2026

clickhouse-gh Bot reviewed Mar 26, 2026

View reviewed changes

Comment thread src/Coordination/KeeperContext.cpp Outdated

Fix

d55c9cd

clickhouse-gh Bot reviewed Mar 26, 2026

View reviewed changes

Comment thread src/Server/KeeperTCPHandler.cpp Outdated

tidy

993c319

clickhouse-gh Bot reviewed Mar 26, 2026

View reviewed changes

Comment thread src/Coordination/CoordinationSettings.cpp Outdated

Merge remote-tracking branch 'origin/master' into hot

4e629c8

al13n321 requested a review from antonio2368 March 27, 2026 22:13

Merge branch 'master' into hot

c1768a5

antonio2368 self-assigned this Mar 30, 2026

antonio2368 reviewed Mar 31, 2026

View reviewed changes

al13n321 added 3 commits March 31, 2026 22:23

Merge remote-tracking branch 'origin/master' into hot

7509d2a

Comments

6ca8e97

Merge remote-tracking branch 'origin/master' into hot

86b44bd

clickhouse-gh Bot reviewed Apr 1, 2026

View reviewed changes

Comment thread tests/integration/test_keeper_dynamic_settings/test.py

al13n321 and others added 2 commits April 7, 2026 13:41

Merge remote-tracking branch 'origin/master' into hot

a52ae73

Merge branch 'master' into hot

5e84292

clickhouse-gh Bot reviewed Apr 8, 2026

View reviewed changes

Comment thread tests/integration/test_keeper_dynamic_settings/test.py

Rename getDynamicSettings -> getCoordinationSettings, getCoordination…

05140e2

…Settings -> getFixedSettings

Merge branch 'master' into hot

0482c5c

antonio2368 approved these changes Apr 9, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into hot

16306c3

clickhouse-gh Bot reviewed Apr 9, 2026

View reviewed changes

Comment thread src/Coordination/KeeperContext.cpp

al13n321 enabled auto-merge April 9, 2026 19:35

Merge remote-tracking branch 'origin/master' into hot

43fb80e

clickhouse-gh Bot 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 Apr 10, 2026

antonio2368 mentioned this pull request Apr 10, 2026

Fix flaky timeouts in union race condition tests #102372

Merged

1 task

Merge remote-tracking branch 'origin/master' into hot

341039b

clickhouse-gh Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread tests/integration/test_keeper_dynamic_settings/test.py

al13n321 added this pull request to the merge queue Apr 11, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 11, 2026

al13n321 added this pull request to the merge queue Apr 11, 2026

Merged via the queue into master with commit 52c89f4 Apr 11, 2026
157 of 164 checks passed

al13n321 deleted the hot branch April 11, 2026 02:02

robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 11, 2026

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

al13n321 commented Mar 26, 2026 • edited by robot-clickhouse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Version info

Uh oh!

clickhouse-gh Bot commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Tests

ClickHouse Rules

Final Verdict

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antonio2368 Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

al13n321 Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonio2368 Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

al13n321 Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

antonio2368 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

al13n321 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

antonio2368 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

al13n321 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexey-milovidov commented Apr 7, 2026

Uh oh!

alexey-milovidov commented Apr 8, 2026

Uh oh!

Uh oh!

alexey-milovidov commented Apr 8, 2026

Uh oh!

antonio2368 commented Apr 9, 2026

Uh oh!

Uh oh!

antonio2368 commented Apr 10, 2026

Uh oh!

Uh oh!

clickhouse-gh Bot commented Apr 10, 2026

LLVM Coverage Report

Uh oh!

al13n321 commented Apr 11, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

al13n321 commented Mar 26, 2026 •

edited by robot-clickhouse

Loading

clickhouse-gh Bot commented Mar 26, 2026 •

edited

Loading

al13n321 Apr 1, 2026 •

edited

Loading