Don't handle timeout_overflow_mode='break' in ReplicatedMergeTreeSink::waitForQuorum#107760
Conversation
clickhouse-gh
Bot
added
the
pr-not-for-changelog
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| INSERT INTO quorum_break_r1 | ||
| SETTINGS insert_quorum = 2, insert_quorum_parallel = 0, insert_quorum_timeout = 6000000, max_execution_time = 3, timeout_overflow_mode = 'break', async_insert = 0, insert_keeper_fault_injection_probability = 0 | ||
| VALUES (1); -- { serverError TIMEOUT_EXCEEDED } | ||
| SETTINGS insert_quorum = 2, insert_quorum_parallel = 0, insert_quorum_timeout = 3000, max_execution_time = 300, timeout_overflow_mode = 'break', async_insert = 0, insert_keeper_fault_injection_probability = 0 |
There was a problem hiding this comment.
This setting means the break case never exercises the behavior this PR changes. insert_quorum_timeout = 3000 bounds the wait at about 3 seconds, while max_execution_time = 300 leaves checkTimeLimit returning true for the whole wait, so this test would still pass if the new waitForQuorum call ignored no false return at all.
Please add focused coverage where max_execution_time expires while the query is already waiting for quorum and insert_quorum_timeout is still in the future, then assert that the query continues to the quorum timeout and reports UNKNOWN_STATUS_OF_INSERT. A small shell test like 04338_kill_quorum_insert can make this deterministic by starting the quorum INSERT, waiting until it is blocked in the quorum wait, and only then observing the result.
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered by tests: 7/7 (100.00%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 8 line(s) · Uncovered code |
robot-ch-test-poll
added
the
pr-synced-to-cloud
3 participants
Changelog category (leave one):
#107513 caused failures in CI, and we decided to reconsider the behaviour with
timeout_overflow_mode='break'.Version info
26.6.1.976