Add ZooKeeper connection log by antaljanosbenjamin · Pull Request #79494 · ClickHouse/ClickHouse

antaljanosbenjamin · 2025-04-23T16:41:38Z

Changelog category (leave one):

New Feature

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

Add zookeeper_connection_log system table to store historical information about ZooKeeper connections.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

clickhouse-gh · 2025-04-23T16:42:05Z

Workflow [PR], commit [34f7deb]

nikitamikhaylov · 2025-04-23T16:43:51Z

Why not to add a new type to existing system.zookeeper_log ?

Algunenano · 2025-04-23T16:46:17Z

Why not to add a new type to existing system.zookeeper_log ?

Zookeeper log is unusable in the long term due to the high volume of messages it stores.

antaljanosbenjamin · 2025-04-24T13:44:13Z

Yes, exactly what @Algunenano said. This should be a much more lightweight table that we might even enable in our cloud. Recently both @vdimir run into issues where this would have been a valuable information.

antaljanosbenjamin · 2025-04-25T09:55:47Z

Github suggested me to ask Copilot for a review. I cannot stand not to do so. Let's see how good is that :D

Copilot

Pull Request Overview

This PR adds a new system table for logging ZooKeeper connection information and integrates it into both the Context and SystemLog machinery while also replacing legacy print statements in integration helpers with proper logging.

Replaces print() calls with logging.debug in helper modules
Introduces ZooKeeperConnectionLog (header and implementation) and integrates connection/disconnection event logging in Context and ZooKeeper-related modules
Updates related components (SystemLog, ZooKeeper classes, etc.) to support the new logging mechanism

Reviewed Changes

Copilot reviewed 18 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/integration/helpers/cluster.py	Replaces print statements with logging.debug calls for better logging consistency
src/Storages/System/StorageSystemZooKeeperConnection.cpp	Updates retrieval of enabled feature flags and uses the new default ZooKeeper name from ZooKeeperConnectionLog
src/Interpreters/ZooKeeperConnectionLog.{h,cpp}	Introduces new system log element and logging API for ZooKeeper connection events
src/Interpreters/SystemLog.{h,cpp}	Integrates the new ZooKeeperConnectionLog into the system logs
src/Interpreters/Context.{h,cpp}	Adds and updates functions to log ZooKeeper connection events and to use the default ZooKeeper name consistently
src/Common/ZooKeeper/{ZooKeeperImpl.h, ZooKeeper.h, ZooKeeper.cpp}	Improves const correctness and minor naming clarifications
src/Common/SystemLogBase.{h,cpp}	Maps the new ZooKeeperConnectionLogElement in the system log base

Files not reviewed (5)

programs/server/config.d/zookeeper_connection_log.xml: Language not supported
tests/integration/log.txt: Language not supported
tests/integration/test_reload_auxiliary_zookeepers/configs/config.xml: Language not supported
tests/integration/test_reload_auxiliary_zookeepers/configs/users.xml: Language not supported
tests/integration/test_zookeeper_connection_log/configs/auxiliary_zookeepers.xml: Language not supported

Comments suppressed due to low confidence (1)

src/Interpreters/Context.cpp:4365

[nitpick] The parameter name 'zk_conection_log' seems to have a typo; consider renaming it to 'zk_connection_log' for clarity and consistency.

std::shared_ptr<ZooKeeperConnectionLog> zk_conection_log,

tests/integration/helpers/cluster.py

antaljanosbenjamin · 2025-05-09T15:56:04Z

src/Interpreters/ZooKeeperConnectionLog.cpp

+        element.port = static_cast<UInt16>(Poco::NumberParser::parseUnsigned(host_port.substr(offset + 1)));
+    }
+    element.index = zookeeper.getConnectedHostIdx();
+    element.keeper_api_version = 0;


The same is done in system.zookeeper_connection. I tried to expose the API version, but for some reason it sometimes returned non-sense values, therefore I decided to do the same as in zookeeper_connection as it is not a very important information (enabled feature flags contain the necessary information).

…order By changing the normal zookeeper config first we can ensure that the order of events in `zookeeper_connection_log` will match the expected one even in cases when the config reload is triggered automatically while only one of the files are modified.

antaljanosbenjamin

One more thing I just realized: we don't log disconnected events in case of server shutdown/restart. However because of the initialization events it should be obvious. That can be added later.

Avogar

Looks great!

antaljanosbenjamin · 2025-05-20T09:43:14Z

I have to fix the test, it failed in private for some reason.

clickhouse-gh · 2025-06-24T13:18:37Z

Dear @Avogar, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

…n-log

clickhouse-gh · 2025-08-11T14:18:33Z

Workflow [PR], commit [f35e504]

…ializing `ZooKeeperConnectionLog`

antaljanosbenjamin · 2025-08-11T23:41:32Z

@Avogar I made the test pass also in private with f35e504, so I think the PR should be ready to be merged. Could you please take another look?

antaljanosbenjamin · 2025-08-12T12:07:56Z

02443_detach_attach_partition is flaky
test_async_connect_to_multiple_ips/test.py::test is also flaky
Fuzzer issue

Making this green manually.

clickhouse-gh bot added the pr-feature Pull request with new product feature label Apr 23, 2025

antaljanosbenjamin requested a review from Copilot April 25, 2025 09:55

Copilot AI reviewed Apr 25, 2025

View reviewed changes

tests/integration/helpers/cluster.py Outdated Show resolved Hide resolved

antaljanosbenjamin added 9 commits May 9, 2025 12:22

Add ZooKeeper connection log

a0aa862

Simplify zookeeper connection logging

5aa0508

Add reason column to ZooKeeper connection log

5de7190

Remove unnecessary configs

5b12f1d

Use logging instead of print

3467a0a

Add test

e60f7b4

Make test repeatable

121c6a9

Fix typo

b111c38

Fix logging

fe01c37

antaljanosbenjamin force-pushed the zookeeper-connection-log branch from 6d7123d to fe01c37 Compare May 9, 2025 12:42

antaljanosbenjamin marked this pull request as ready for review May 9, 2025 12:46

antaljanosbenjamin commented May 9, 2025

View reviewed changes

antaljanosbenjamin added 5 commits May 9, 2025 21:21

Make test simpler by preventing automatic config reloads

1acb6ee

Avoid time.sleep

92d95d9

Make index not nullable

8bed8f7

Add docs

a796c80

Remove extra empty line

34f7deb

Avogar self-assigned this May 14, 2025

Avogar approved these changes May 16, 2025

View reviewed changes

clickhouse-gh bot unassigned Avogar Jun 24, 2025

Merge remote-tracking branch 'origin/master' into zookeeper-connectio…

094a009

…n-log

antaljanosbenjamin assigned Avogar Aug 11, 2025

antaljanosbenjamin added 3 commits August 11, 2025 16:40

Make with_replace_config work well in case of exceptions

b3ac33a

Formatting

3de58ce

Update test with newly enabled MULTI_WATCHES

583693f

antaljanosbenjamin force-pushed the zookeeper-connection-log branch from 3972702 to 4ec7cff Compare August 11, 2025 22:34

Add log entries for already existing ZooKeeper connections after init…

f35e504

…ializing `ZooKeeperConnectionLog`

antaljanosbenjamin force-pushed the zookeeper-connection-log branch from 4ec7cff to f35e504 Compare August 11, 2025 22:37

antaljanosbenjamin requested a review from Avogar August 11, 2025 23:40

Avogar approved these changes Aug 12, 2025

View reviewed changes

antaljanosbenjamin added this pull request to the merge queue Aug 12, 2025

Merged via the queue into master with commit f08c6da Aug 12, 2025
120 of 123 checks passed

antaljanosbenjamin deleted the zookeeper-connection-log branch August 12, 2025 12:22

robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Aug 12, 2025

Conversation

antaljanosbenjamin commented Apr 23, 2025 • edited by fm4v 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 to CHANGELOG.md):

Documentation entry for user-facing changes

Uh oh!

clickhouse-gh bot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikitamikhaylov commented Apr 23, 2025

Uh oh!

Algunenano commented Apr 23, 2025

Uh oh!

antaljanosbenjamin commented Apr 24, 2025

Uh oh!

antaljanosbenjamin commented Apr 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

antaljanosbenjamin May 9, 2025

Choose a reason for hiding this comment

Uh oh!

antaljanosbenjamin left a comment

Choose a reason for hiding this comment

Uh oh!

Avogar left a comment

Choose a reason for hiding this comment

Uh oh!

antaljanosbenjamin commented May 20, 2025

Uh oh!

clickhouse-gh bot commented Jun 24, 2025

Uh oh!

clickhouse-gh bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antaljanosbenjamin commented Aug 11, 2025

Uh oh!

antaljanosbenjamin commented Aug 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

antaljanosbenjamin commented Apr 23, 2025 •

edited by fm4v

Loading

clickhouse-gh bot commented Apr 23, 2025 •

edited

Loading

clickhouse-gh bot commented Aug 11, 2025 •

edited

Loading