Skip to content

Split Keeper replication out of ReplicatedAccessStorage for shared use#81245

Merged
thevar1able merged 1 commit intomasterfrom
pullout-replication-from-replicated-access
Jun 16, 2025
Merged

Split Keeper replication out of ReplicatedAccessStorage for shared use#81245
thevar1able merged 1 commit intomasterfrom
pullout-replication-from-replicated-access

Conversation

@kcmannem
Copy link
Copy Markdown
Collaborator

@kcmannem kcmannem commented Jun 3, 2025 •
edited
Loading

A slight refactor for ReplicatedAccessStorage. This decouples the replication logic into a separate ZooKeeperReplicator class so that we can share it between other AccessStorages.

Also updates the error messaging to include the Access Storage which throws the exception, to identify more clearly the source. Needed when multiple storages can use keeper replication.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 3, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@kcmannem kcmannem requested review from pufit and thevar1able June 3, 2025 18:56
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 3, 2025 •
edited
Loading

Workflow [PR], commit [39eb90b]

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 3, 2025
@kcmannem kcmannem force-pushed the pullout-replication-from-replicated-access branch 3 times, most recently from 1a409a5 to 2962151 Compare June 4, 2025 17:15
@kcmannem kcmannem mentioned this pull request Jun 4, 2025
Closed
@kcmannem
Copy link
Copy Markdown
Collaborator Author

kcmannem commented Jun 4, 2025

CI failing on master as well for test_replicated_users unrelated to this change.

Tagging issue: #81319

@kcmannem
Copy link
Copy Markdown
Collaborator Author

kcmannem commented Jun 4, 2025

Here's the direct diff between ReplicatedAccessStorage and ZooKeeperReplicator to spot any missed lines: https://www.diffchecker.com/kIk399ta/

@thevar1able thevar1able changed the title pull keeper replication out of ReplicatedAccessStorage for shared use Split Keeper replication out of ReplicatedAccessStorage for shared use Jun 5, 2025
@thevar1able thevar1able changed the title Split Keeper replication out of ReplicatedAccessStorage for shared use Split Keeper replication out of ReplicatedAccessStorage for shared use Jun 5, 2025
@kcmannem kcmannem force-pushed the pullout-replication-from-replicated-access branch from 9669cd7 to 39eb90b Compare June 5, 2025 13:57
@kcmannem
Copy link
Copy Markdown
Collaborator Author

kcmannem commented Jun 5, 2025

feels like the integration test failing is a flake. Ran locally on my branch with:
sudo ./runner --binary $HOME/work/ClickHouse/cmake-build-debug/programs/clickhouse --count 5 --random 'test_create_user_and_login' -- -ss

and its all green:

test_create_user_and_login/test.py::test_grant_create_user[3-5] Copy common default production configuration from /clickhouse-config. Files: config.xml, users.xml
PASSED
test_create_user_and_login/test.py::test_login[3-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user[5-5] PASSED
test_create_user_and_login/test.py::test_login[2-5] PASSED
test_create_user_and_login/test.py::test_login[1-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user_xml[4-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user[4-5] PASSED
test_create_user_and_login/test.py::test_grant_create_user[2-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user[2-5] PASSED
test_create_user_and_login/test.py::test_grant_create_user[4-5] PASSED
test_create_user_and_login/test.py::test_grant_create_user[5-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user[3-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user_xml[3-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user_xml[1-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user_xml[5-5] PASSED
test_create_user_and_login/test.py::test_login[4-5] PASSED
test_create_user_and_login/test.py::test_grant_create_user[1-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user_xml[2-5] PASSED
test_create_user_and_login/test.py::test_login[5-5] PASSED
test_create_user_and_login/test.py::test_login_as_dropped_user[1-5] PASSED

========================================================================================================= 20 passed in 53.31s ==========================================================================================================

@pufit
Copy link
Copy Markdown
Member

pufit commented Jun 13, 2025

LGTM. @thevar1able, what do you think?

thevar1able
thevar1able approved these changes Jun 16, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@thevar1able thevar1able left a comment

Choose a reason for hiding this comment

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

LGTM.

Performance comparison failure is unrelated.

@thevar1able thevar1able added this pull request to the merge queue Jun 16, 2025
Merged via the queue into master with commit d58a007 Jun 16, 2025
228 of 240 checks passed
@thevar1able thevar1able deleted the pullout-replication-from-replicated-access branch June 16, 2025 01:41
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 16, 2025
@rschu1ze
Copy link
Copy Markdown
Member

rschu1ze commented Jun 16, 2025

@kcmannem The CLA bot complained. I guess it's technically not required that ClickHouse Inc. employees sign the CLA but to avoid that people repeatedly ask in future, it is advised to do so.

This was referenced Jun 17, 2025
Closed
Merged
@tavplubix
Copy link
Copy Markdown
Member

tavplubix commented Jun 17, 2025

feels like the integration test failing is a flake. Ran locally on my branch with:
sudo ./runner --binary $HOME/work/ClickHouse/cmake-build-debug/programs/clickhouse --count 5 --random 'test_create_user_and_login' -- -ss
and its all green:

@kcmannem the failure was actually related to the changes in this PR. "It works on my machine" != "it works".

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

Reviewers

@thevar1able thevar1able thevar1able approved these changes

@pufit pufit pufit approved these changes

Assignees

@pufit pufit

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@kcmannem @CLAassistant @pufit @rschu1ze @tavplubix @thevar1able @robot-clickhouse-ci-1