New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize cluster node list change but DirectoryMonitor can't sense it #42826
Optimize cluster node list change but DirectoryMonitor can't sense it #42826
Conversation
Can you help with the review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR had been lost, but it looks good to me (except for minor comments, and you will need to rebase on top master, since there was some refactoring in this place), I hope that you will have to finish it!
And it definitely needs an integration test.
@azat ok, I will continue to complete it if necessary |
5908aa8
to
6c40a1c
Compare
@azat I need help. During the process of writing integration tests, if nodes are dynamically brought online or offline |
I guess you don't actually need to change their online/offline status, change But you want to change online/offline, you can use |
6c40a1c
to
8867c55
Compare
Thank you very much for your reply, i have added an integration test, can you help me review it? |
tests/integration/test_distributed_async_insert_for_node_changes/test.py
Outdated
Show resolved
Hide resolved
src/Storages/Distributed/DistributedAsyncInsertDirectoryQueue.cpp
Outdated
Show resolved
Hide resolved
8867c55
to
0a09bcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay (was on vacation), LGTM
This is an automated comment for commit beabc52 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
@zhongyuankai can you please fix this |
@azat Thanks for your reply, I will fix it. |
4f35a0f
to
e384389
Compare
Still an issue, but let's just change the changelog entry to |
New test fails |
@azat Already fixed, this failure should have nothing to do with changes. |
tests/integration/test_distributed_async_insert_for_node_changes/configs/remote_servers.xml
Show resolved
Hide resolved
Looks like internal CI issue - #56108 |
tests/integration/test_distributed_async_insert_for_node_changes/configs/remote_servers.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Now someone from the official team should take a look, let's ask @alexey-milovidov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor change and I think it is good to go! Thank you @azat for catching issues and doing the review!
7b0f8d4
into
ClickHouse:master
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Make inserts into distributed tables handle updated cluster configuration properly. When the list of cluster nodes is dynamically updated, the Directory Monitor of the distribution table cannot sense the new node, and the Directory Monitor must be re-noded to sense it.