Skip to content

add new system.zookeeper_watches table#99277

Open
Diskein wants to merge 9 commits intomasterfrom
dk-system-zookeeper-watches
Open

add new system.zookeeper_watches table#99277
Diskein wants to merge 9 commits intomasterfrom
dk-system-zookeeper-watches

Conversation

@Diskein
Copy link
Contributor

@Diskein Diskein commented Mar 11, 2026

Implements #86736

Changelog category (leave one):

  • New Feature

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

Users can now see zookeeper's watches issued by clickhouse-server using the new system.zookeeper_watches table.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Contributor

clickhouse-gh bot commented Mar 11, 2026

Workflow [PR], commit [2962b96]

Summary:

job_name test_name status info comment
Integration tests (amd_asan, targeted) failure
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[4-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[5-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[3-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[9-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[1-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[6-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[8-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[2-10] FAIL cidb
test_move_ttl_broken_compatibility/test.py::test_bc_compatibility[7-10] FAIL cidb
test_s3_aws_sdk_has_slightly_unreliable_behaviour/test.py::test_dataloss[7-10] FAIL cidb
8 more test cases not shown

@clickhouse-gh clickhouse-gh bot added the pr-feature Pull request with new product feature label Mar 11, 2026
@Diskein Diskein force-pushed the dk-system-zookeeper-watches branch from 8ab404c to 7baa1ae Compare March 12, 2026 18:31
@Diskein Diskein force-pushed the dk-system-zookeeper-watches branch from 7baa1ae to decab89 Compare March 16, 2026 19:51
@Diskein
Copy link
Contributor Author

Diskein commented Mar 16, 2026

create_time here is the time when we got response from our zookeeper and added callback on client for this watch

@Diskein Diskein marked this pull request as ready for review March 16, 2026 20:24
@Diskein Diskein force-pushed the dk-system-zookeeper-watches branch 2 times, most recently from f930d75 to 6ddb818 Compare March 17, 2026 17:15
@Michicosun Michicosun self-assigned this Mar 17, 2026
@Diskein Diskein force-pushed the dk-system-zookeeper-watches branch from 3261f61 to 2962b96 Compare March 18, 2026 16:03
@clickhouse-gh
Copy link
Contributor

clickhouse-gh bot commented Mar 18, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.80% 83.80% +0.00%
Functions 23.90% 23.90% +0.00%
Branches 76.30% 76.30% +0.00%

PR changed lines: PR changed-lines coverage: 92.82% (168/181, 0 noise lines excluded)
Diff coverage report
Uncovered code

@Diskein
Copy link
Contributor Author

Diskein commented Mar 23, 2026

@Diskein
Copy link
Contributor Author

Diskein commented Mar 23, 2026

@Michicosun could you take a look pls

Copy link
Member

@Michicosun Michicosun left a comment

Choose a reason for hiding this comment

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

First Review Notes

namespace Coordination
{

class ZooKeeperWatchesTracker
Copy link
Member

Choose a reason for hiding this comment

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

Basically, this structure duplicates the watches map, which doubles the byte size for watches. Why isn't this information stored not in watches map itself?

for (const auto & event_or_callback : it->second)
{
if (event_or_callback)
{
Copy link
Member

Choose a reason for hiding this comment

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

There is a bug in master; it should not remove all watches based on the triggered path; it should remove only the triggered watch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-feature Pull request with new product feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants