S3Queue auxiliary Zookeeper support (not clean) by lesandie · Pull Request #93786 · ClickHouse/ClickHouse

lesandie · 2026-01-09T08:55:00Z

Changelog category (leave one):

Improvement

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

S3Queue auxiliary Zookeeper support using keeper_path setting from s3Queue

This will allow to do workload separation for heavy zk usage modes like unordered, by creating a zk/keeper specifically for some S3Queue tables and creating an S3queue specifying keeper_path with an auxiliary zookeeper like in a ReplicatedMergeTree table:

'auxiliary_zookeeper:/clickhouse/s3queue/my_s3queue_table'

Added integration tests and doc

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

lesandie · 2026-01-09T08:59:16Z

Please @ianton-ru and @kssenii could you final review?.

I added all the comments and changes @ianton-ru proposed here: lesandie#2

and resolved conflicts from here: #81040

ianton-ru · 2026-01-09T09:44:06Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadata.cpp

            zk_retries.retryLoop([&]
            {
-                response = getZooKeeper(log)->tryGet(paths);
+                response = zk_client->tryGet(paths);


As I understand need to get client from getZooKeeper inside retryLoop because retry can be caused by disconnect.

I collapsed it when switching to the cached zk_client, but that did drop the retry behavior 😓

will add back the getZookeeper

ianton-ru · 2026-01-09T09:44:49Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadata.cpp

-        {
-            code = getZooKeeper(log)->tryMulti(remove_requests, remove_responses);
-        });
+        code = zk_client->tryMulti(remove_requests, remove_responses);


Why retryLoop removed here?

same as above. I'll add here the retry but it is better to leave the cached zk_client and not use the getZookeeper() here right? (as it is cleaning nodes). At least it makes sense to me but not 100% sure.

clickhouse-gh · 2026-01-09T10:44:50Z

Workflow [PR], commit [58eb559]

Summary: ❌

job_name	test_name	status	info
Fast test		failure
	03802_float_to_enum_overflow	FAIL	cidb
Docs check		dropped
Build (amd_debug)		dropped
Build (amd_asan)		dropped
Build (amd_tsan)		dropped
Build (amd_msan)		dropped
Build (amd_ubsan)		dropped
Build (amd_binary)		dropped
Build (arm_asan)		dropped
Build (arm_binary)		dropped

Copilot

Pull request overview

This PR adds support for auxiliary ZooKeeper clusters in S3Queue and AzureQueue tables through the keeper_path setting. This feature enables workload separation for heavy ZooKeeper usage patterns by allowing tables to specify a dedicated keeper instance.

Changes:

Extended keeper_path setting to support auxiliary ZooKeeper name prefix (e.g., zookeeper2:/clickhouse/s3queue/table)
Threaded zookeeper_name through the metadata, file metadata, and bucket holder classes
Added validation for unknown auxiliary ZooKeeper names
Updated documentation with usage examples

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/integration/test_storage_s3_queue/test_0.py	Added integration tests for auxiliary ZooKeeper support and missing configuration handling
tests/integration/test_storage_s3_queue/configs/zookeeper.xml	Added auxiliary ZooKeeper configuration for tests
src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.h	Added getter for ZooKeeper name and updated constructor signature
src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp	Implemented ZooKeeper name extraction and path parsing logic
src/Storages/ObjectStorageQueue/ObjectStorageQueueUnorderedFileMetadata.h	Added zookeeper_name parameter to constructor and static methods
src/Storages/ObjectStorageQueue/ObjectStorageQueueUnorderedFileMetadata.cpp	Updated to pass zookeeper_name to ZooKeeper client calls
src/Storages/ObjectStorageQueue/ObjectStorageQueueSource.cpp	Modified to use metadata's getZooKeeper() method instead of static calls
src/Storages/ObjectStorageQueue/ObjectStorageQueueOrderedFileMetadata.h	Added zookeeper_name parameter throughout the class
src/Storages/ObjectStorageQueue/ObjectStorageQueueOrderedFileMetadata.cpp	Updated all ZooKeeper client calls to pass zookeeper_name
src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadataFactory.h	Added zookeeper_name parameter to getOrCreate and remove methods
src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadataFactory.cpp	Implemented metadata key composition including zookeeper_name
src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadata.h	Added zookeeper_name field and updated getZooKeeper method signature
src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadata.cpp	Updated all ZooKeeper operations to use auxiliary keeper when specified
src/Storages/ObjectStorageQueue/ObjectStorageQueueIFileMetadata.h	Added zookeeper_name field to base file metadata class
src/Storages/ObjectStorageQueue/ObjectStorageQueueIFileMetadata.cpp	Updated ZooKeeper client calls to use zookeeper_name
docs/en/engines/table-engines/integrations/s3queue.md	Documented auxiliary ZooKeeper support in keeper_path setting

Copilot · 2026-01-12T12:28:32Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueOrderedFileMetadata.cpp

-                return {false, FileStatus::State::Processed};
+                Coordination::Requests requests;
+                std::vector<std::string> paths{processed_node_path, failed_node_path};
+                zkutil::ZooKeeper::MultiTryGetResponse responses = ObjectStorageQueueMetadata::getZooKeeper(log, zookeeper_name)->tryGet(paths);


The multi-get ZooKeeper request at line 486 is not wrapped in a retry loop like similar operations elsewhere in the codebase. If this request encounters a transient failure (session expiration, network issue), it will not be retried, potentially causing inconsistent state. Wrap this call in getKeeperRetriesControl(log).retryLoop([&] { ... }) similar to other ZooKeeper operations in this file.

Suggested change

zkutil::ZooKeeper::MultiTryGetResponse responses = ObjectStorageQueueMetadata::getZooKeeper(log, zookeeper_name)->tryGet(paths);

zkutil::ZooKeeper::MultiTryGetResponse responses;

getKeeperRetriesControl(log).retryLoop([&]

{

responses = ObjectStorageQueueMetadata::getZooKeeper(log, zookeeper_name)->tryGet(paths);

});

Same issue pointed out by @ianton-ru ... so it seems we should wrap in retryLoop all getZookeeper() just to be consistent even when method is cleaning nodes (dropping a queue) right?

Copilot · 2026-01-12T12:28:32Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueOrderedFileMetadata.cpp

+                if (getMaxProcessedNode(node_metadata, &processed_node_stat_local, processed_node_path, log, zookeeper_name))
+                {
+                    processed_node_stat = processed_node_stat_local;
+                    bool failed_node_exists = ObjectStorageQueueMetadata::getZooKeeper(log, zookeeper_name)->exists(failed_node_path);


The exists() check at line 525 is not wrapped in a retry loop. If this encounters a transient ZooKeeper failure, it could incorrectly conclude the node doesn't exist. Wrap this call in getKeeperRetriesControl(log).retryLoop([&] { ... }) for consistency with other ZooKeeper operations.

Suggested change

bool failed_node_exists = ObjectStorageQueueMetadata::getZooKeeper(log, zookeeper_name)->exists(failed_node_path);

bool failed_node_exists = false;

getKeeperRetriesControl(log).retryLoop([&]

{

failed_node_exists = ObjectStorageQueueMetadata::getZooKeeper(log, zookeeper_name)->exists(failed_node_path);

});

Copilot · 2026-01-12T12:28:33Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueOrderedFileMetadata.cpp

+    std::optional<std::string> processed_node_hive_partitioning_path;
+    if (is_path_with_hive_partitioning)
+        processed_node_hive_partitioning_path = std::filesystem::path(processed_node_path_) / getHivePart(path);
+    auto processed_file_info = getLastProcessedFile(
+        &processed_node_stat,
+        processed_node_path_,
+        path,
+        processed_node_hive_partitioning_path,
+        std::nullopt,
+        log,
+        zookeeper_name);


The removed code at lines 647-650 (constructing processed_node_hive_partitioning_path) appears to have been replaced with a different logic block that duplicates this same logic. The new code in lines 648-650 recreates the same variable and value that was previously computed. This refactor doesn't change behavior but the diff structure makes it appear more significant than it is.

lesandie · 2026-01-14T10:50:26Z

@kssenii and @ianton-ru I merged from master lastest changes

should I commit copilot suggestions or should copilot review again after merging master and applying some changes?.

The previous ones look to me good, just using the retryLoop with lambda. The last comment about processed_node_hive_partitioning_path it is related to the hive modifications. I'm not sure how to proceed, but is also seems the added params to getLastProcessedFile are ok.

std::optional<std::string> processed_node_hive_partitioning_path;

is optional so if hive partitioning is activated then params provided to method are good

Am I correct?

…ault.

lesandie · 2026-01-17T09:10:14Z

Hi @kssenii seems like the buzzhouse tests are not passing. The CI log itself marks it flaky and links to issue #93861. It doesn’t appear tied to current s3queue changes. Other than this PR seems good now after adding all comments.

lesandie · 2026-01-21T08:29:01Z

Hi @kssenii

After #94321 is merged, I’ll need to review and resolve any conflicts. I’m happy to do that, but I wanted to check with you first: should I wait for the changes in #94412 as well and then proceed, so we can avoid resolving conflicts multiple times?

I understand that the partitioning feature should be stabilized before we introduce another feature like this one. I’m completely fine waiting and following your preferred order of changes. I'm just trying to help and keep things moving in line with your plan.

kssenii · 2026-01-21T11:35:51Z

src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.h

    const auto & getFormatName() const { return configuration->format; }

    const fs::path & getZooKeeperPath() const { return zk_path; }
+    const String & getZooKeeperName() const { return zookeeper_name; }


This method seems unused? I see only same method from queue metadata is used

kssenii · 2026-01-21T11:36:48Z

src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.h

        const ObjectStorageQueueSettings & queue_settings,
-        UUID database_uuid = UUIDHelpers::Nil);
+        UUID database_uuid = UUIDHelpers::Nil,
+        String * zookeeper_name_out = nullptr);


Suggested change

String * zookeeper_name_out = nullptr);

String * result_zookeeper_name = nullptr);

kssenii · 2026-01-21T11:44:15Z

src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp

+        bool starts_with_slash = !configured_path.empty() && configured_path.front() == '/';
+        bool has_keeper_prefix = zkutil::extractZooKeeperName(configured_path) != zkutil::DEFAULT_ZOOKEEPER_NAME;
+
+        if (has_keeper_prefix || starts_with_slash)


Why avoid zk_path_prefix with auxiliary zk? Seems we still could use it if keeper_path is relative

kssenii · 2026-01-21T12:02:41Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueOrderedFileMetadata.h

    bool released = false;
    bool finished = false;
    LoggerPtr log;
+    std::string zookeeper_name;


Let's put it inside bucket info struct?

kssenii · 2026-01-21T12:04:00Z

src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadata.cpp

        zk_retries.retryLoop([&]
        {
-            code = getZooKeeper(log)->tryMulti(remove_requests, remove_responses);
+            code = zk_client->tryMulti(remove_requests, remove_responses);


We should get zk client from getZooKeeper to make sure it is reinitialized on next retry if session expired

kssenii · 2026-01-21T12:14:34Z

should I wait for the changes in #94412 as well and then proceed, so we can avoid resolving conflicts multiple times?

Hi! This is up to you, I will merge whichever of them is green and approved first, and that one (mine) currently has a failing test, I will likely fix it today. Did not have time to review your PR before now, not because I was planning to merge something else first.

This reverts commit f335783, reversing changes made to e76de8c.

lesandie · 2026-01-22T12:52:06Z

should I wait for the changes in #94412 as well and then proceed, so we can avoid resolving conflicts multiple times?

Hi! This is up to you, I will merge whichever of them is green and approved first, and that one (mine) currently has a failing test, I will likely fix it today. Did not have time to review your PR before now, not because I was planning to merge something else first.

Hi @kssenii thanks for your response!,

I acted to fast and applied your comments and changes before merging latest master changes, so I have to do that again 🤦🏼 .

I will align my code with the latests bucketing changes you've merged.

…per_name next to zookeeper_path in any related APIs for readability.

lesandie · 2026-01-23T09:30:44Z

Hi @kssenii,

Some tests do fails but IDK if they are related or not. I compiled/tested locally and it works.

Could you do one final review?

Thanks!!!

lesandie · 2026-01-26T10:27:52Z

Hi @kssenii it seems this test is failing:

https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=93786&sha=58eb559ddb9973d941e52603ffbabe4081ac7187&name_0=PR&name_1=Fast+test&name_2=Tests

Float to Enum overflow ... seems not related

kssenii · 2026-01-26T12:54:29Z

Hi, it looks like you incorrectly merged master, because of lot of unrelated changes appeared in the PR diff

lesandie · 2026-01-26T15:07:32Z

Hi, it looks like you incorrectly merged master, because of lot of unrelated changes appeared in the PR diff

Sorry! I see, I will close this PR and create a new one only with the actual changes

Fix compilation errors

5a6afc1

lesandie changed the title ~~Fix compilation errors~~ S3Queue auxiliary Zookeeper support Jan 9, 2026

ianton-ru reviewed Jan 9, 2026

View reviewed changes

kssenii self-assigned this Jan 9, 2026

kssenii added the can be tested Allows running workflows for external contributors label Jan 9, 2026

clickhouse-gh bot added the pr-improvement Pull request with some product improvements label Jan 9, 2026

retryLoop / getZookeeper() readded

23c61bb

kssenii requested a review from Copilot January 12, 2026 12:27

Copilot AI reviewed Jan 12, 2026

View reviewed changes

lesandie added 3 commits January 14, 2026 12:19

Merge remote-tracking branch 'origin' into s3queue_aux_zookeeper

12c0c43

Fix test_0.py, only add the prefix when the ZooKeeper name is non-def…

0b2a2d2

…ault.

Fix indentation

6597977

kssenii reviewed Jan 21, 2026

View reviewed changes

lesandie added 4 commits January 21, 2026 22:11

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

42e6b6b

applied comments from @kssenii

e76de8c

Merge branch 'master' into s3queue_aux_zookeeper

f335783

Revert "Merge branch 'master' into s3queue_aux_zookeeper"

1a48b9e

This reverts commit f335783, reversing changes made to e76de8c.

lesandie added 2 commits January 22, 2026 15:49

Integrated auxiliary zookeper changes with bucketing and moved zookee…

a8e8f66

…per_name next to zookeeper_path in any related APIs for readability.

Aux zookeeper + bucketing updates for ObjectStorageQueue

a4734e4

Merge branch 'master'

58eb559

lesandie marked this pull request as draft January 26, 2026 15:57

lesandie mentioned this pull request Jan 26, 2026

S3Queue auxiliary Zookeeper support #95203

Merged

1 task

lesandie changed the title ~~S3Queue auxiliary Zookeeper support~~ S3Queue auxiliary Zookeeper support (not clean) Jan 28, 2026

lesandie closed this Jan 28, 2026

	String * zookeeper_name_out = nullptr);
	String * result_zookeeper_name = nullptr);

Conversation

lesandie commented Jan 9, 2026 • edited 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):

Documentation entry for user-facing changes

Uh oh!

lesandie commented Jan 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesandie Jan 9, 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!

clickhouse-gh bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

lesandie commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesandie commented Jan 17, 2026

Uh oh!

lesandie commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kssenii commented Jan 21, 2026

Uh oh!

lesandie commented Jan 22, 2026

Uh oh!

lesandie commented Jan 23, 2026

Uh oh!

lesandie commented Jan 26, 2026

Uh oh!

kssenii commented Jan 26, 2026

Uh oh!

lesandie commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lesandie commented Jan 9, 2026 •

edited

Loading

lesandie Jan 9, 2026 •

edited

Loading

clickhouse-gh bot commented Jan 9, 2026 •

edited

Loading

lesandie commented Jan 14, 2026 •

edited

Loading

lesandie commented Jan 21, 2026 •

edited

Loading