Add drop replica alter support by sundy-li · Pull Request #10679 · ClickHouse/ClickHouse

sundy-li · 2020-05-05T15:06:24Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

New Feature

If the replicated table has 3 replicas, but one replica is crashed and never be back to normal again, we should enable to remove this replica by alter commands.

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add Alter table drop replica replica_name support. This fixes #7080.

Detailed description / Documentation draft:
Already in this commit.
.

filimonov · 2020-05-06T20:12:08Z

It should not be possible to remove active replica this way, only stale/inactive.

filimonov · 2020-05-06T20:13:51Z

#7080

qoega · 2020-05-08T06:40:08Z

src/Storages/StorageReplicatedMergeTree.h

      */
    void drop() override;

+    /** Removes a specific replica from Zookeeper. If replica is local, it works same as `drop` method.


Other code and comments tell it is impossible to drop local replica. Incorrect comment?

Yes. I will fix the comment, the local replica can only drop by DROP TABLE

qoega · 2020-05-08T06:41:01Z

docs/en/sql-reference/statements/alter.md

+ALTER TABLE [db].name DROP REPLICA replica_name;
+```
+
+Queries will remove the replica path in zookeeper, it's useful when you want to decrease your replica factor. It will only drop the inactive replica, and it can't drop local replica, please use `DROP TABLE`.


What happens with data on that replica? It remains on remote server?

Yes, it remains. We can't control that. The main reason we call DROP REPLICA is that we should enable remove the 'dead' replica.

sundy-li · 2020-05-08T08:23:57Z

BTW What's Yandex synchronization check? I can't look into the Details.

I'll look into the Integration tests, seems the Ephemeral node /is_acitve not released when we kill the node_1_2

alexey-milovidov · 2020-05-11T03:52:10Z

What's Yandex synchronization check?

It's about a mirror of ClickHouse source code that is automatically created in bigger internal monorepository. You don't have to worry about this check.

alexey-milovidov · 2020-05-11T03:53:33Z

For somewhat reason, newly added integration test does not pass.

tavplubix · 2020-05-13T11:28:28Z

Maybe it should be implemented as SYSTEM query instead of ALTER?
Something like SYSTEM DROP REPLICA 'replica_name' FROM table_name to remove dead replica of existing table and SYSTEM DROP REPLICA 'replica_name' FROM '/path/to/table/in/zk/' to remove dead replica of some table, which does not exists on the local server.
The second query will be useful to remove garbage from ZooKeeper after the local replica was dropped, but metadata of dead replica are left.

sundy-li · 2020-05-14T02:02:42Z

Maybe it should be implemented as SYSTEM query instead of ALTER?
Something like SYSTEM DROP REPLICA 'replica_name' FROM table_name to remove dead replica of existing table and SYSTEM DROP REPLICA 'replica_name' FROM '/path/to/table/in/zk/' to remove dead replica of some table, which does not exists on the local server.
The second query will be useful to remove garbage from ZooKeeper after the local replica was dropped, but metadata of dead replica are left.

Good idea, I will consider it later. (Now I am just a little busy about work)

amudong · 2020-05-26T16:04:57Z

Maybe it should be implemented as SYSTEM query instead of ALTER?
Something like SYSTEM DROP REPLICA 'replica_name' FROM table_name to remove dead replica of existing table and SYSTEM DROP REPLICA 'replica_name' FROM '/path/to/table/in/zk/' to remove dead replica of some table, which does not exists on the local server.
The second query will be useful to remove garbage from ZooKeeper after the local replica was dropped, but metadata of dead replica are left.

Yes, I have implemented the way to replace the ALTER query with SYSTEM, and support the two calling methods you mentioned. I will commit it later.Also, we want to add some methods like SYSTEM DROP REPLICA 'replica_name', SYSTEM DROP REPLICA 'replica_name' FROM DATABASE 'database_name' and SYSTEM DROP REPLICA 'replica_name' FROM TABLE 'database_name.table_name' .

tavplubix · 2020-06-19T10:39:14Z

docs/en/sql-reference/statements/system.md

+Replicas can be dropped using following syntax:
+
+```sql
+SYSTEM DROP REPLICA replica_name;


'replica_name'

tavplubix · 2020-06-19T10:43:14Z

docs/en/sql-reference/statements/system.md

+SYSTEM DROP REPLICA replica_name FROM TABLE database.table;
+```
+
+Queries will remove the replica path in zookeeper, it's useful when you want to decrease your replica factor. It will only drop the inactive/stale replica, and it can't drop local replica, please use `SYSTEM DROP REPLICA` for that.


If I understand correctly, it's useful to remove metadata of dead replica from ZooKeeper. The right way to decrease replication factor is DROP TABLE.

tavplubix · 2020-06-19T11:06:33Z

src/Parsers/ASTSystemQuery.cpp

    };

+    auto print_drop_replica = [&] {
+        settings.ostr << " " << (settings.hilite ? hilite_identifier : "")


It's not an identifier

tavplubix · 2020-06-19T11:55:10Z

src/Interpreters/InterpreterSystemQuery.cpp

+    else if (!query.database.empty())
+    {
+        auto databases = DatabaseCatalog::instance().getDatabases();
+        auto it = databases.find(query.database);


DatabaseCatalog::instance().getDatabase(query.database)

tavplubix · 2020-06-19T12:09:18Z

src/Interpreters/InterpreterSystemQuery.cpp

+            {
+                storage_replicated->getStatus(status);
+
+                if (status.replica_path.compare(query.replica_zk_path + "/replicas/" + status.replica_name) == 0)


If local table with such replica_path exists and DROP TABLE should be used, then it's better to throw exception right here without call of StorageReplicatedMergeTree::dropReplica(...)

tavplubix · 2020-06-19T12:21:55Z

src/Interpreters/InterpreterSystemQuery.cpp

+    LOG_INFO(log, "Removing replica {}", remote_replica_path);
+    /// It may left some garbage if replica_path subtree are concurently modified
+    auto zookeeper = context.getZooKeeper();
+    zookeeper->tryRemoveRecursive(remote_replica_path);


We should check that replica is not active before removing it. Also we probably should remove it the same way as StorageReplicatedMergeTree::dropReplica(...) does. It can be done by extracting some part of StorageReplicatedMergeTree::dropReplica(...) to static method.

tavplubix · 2020-06-19T12:45:24Z

src/Storages/StorageReplicatedMergeTree.cpp

+        throw Exception("Table was not dropped because ZooKeeper session has expired.", ErrorCodes::TABLE_WAS_NOT_DROPPED);
+
+    if (is_drop_table)
+        replica_is_active_node = nullptr;


Isn't it done in StorageReplicatedMergeTree::shutdown() which calls ReplicatedMergeTreeRestartingThread::partialShutdown()?

Fix style && build

Fix integration-tests

update doc Increase timeout to release the zookeeper Ephemeral nodes Fix code comment use PartitionManager make integrations test passed

fix test_drop_replica fix drop replica '/path/to/zk/' ending in '/' and update doc

fix removeReplicaByZKPath fix bug: add zkpath empty judge fix: rewrite code delete useless code. fix:ast fromat fix bug add test_drop_replica add drop_replica doc add drop databse checkAccess refactor dropReplica update tests add static method StorageReplicatedMergeTree::dropReplicaByZkPath update doc and delete useless code fix conflict fix doc fix doc fix StorageReplicatedMergeTree::dropReplica fix bug delete useless code

Merging #10679

cw9 · 2020-06-27T13:49:26Z

Thanks for adding this!

blinkov added doc-alert pr-feature Pull request with new product feature labels May 5, 2020

sundy-li marked this pull request as ready for review May 6, 2020 04:05

qoega reviewed May 8, 2020

View reviewed changes

alexey-milovidov added the st-waiting-for-fix label May 11, 2020

tavplubix self-assigned this May 12, 2020

alexey-milovidov force-pushed the master branch from 6c77191 to 09b9a30 Compare June 9, 2020 02:04

tavplubix reviewed Jun 19, 2020

View reviewed changes

sundy-li marked this pull request as draft June 20, 2020 12:56

amudong force-pushed the drop_replica2 branch 2 times, most recently from 8b6a31e to 340b300 Compare June 22, 2020 15:27

sundy-li and others added 8 commits June 23, 2020 12:12

Add drop replica alter support

906a43e

fix merge state

d4aa3ce

Fix style && build

Add tests and docs

34df59b

Fix integration-tests

only drop inactive replica

15ad830

update doc Increase timeout to release the zookeeper Ephemeral nodes Fix code comment use PartitionManager make integrations test passed

add SYSTEM DROP REPLICA

0a4af8f

fix: can not drop local table and replace ALTER with SYSTEM

2e6a3ef

fix test_drop_replica fix drop replica '/path/to/zk/' ending in '/' and update doc

rewrite static StorageReplicatedMergeTree::dropReplica

0101947

amudong force-pushed the drop_replica2 branch from 340b300 to 0101947 Compare June 23, 2020 04:19

amudong added 2 commits June 23, 2020 12:24

format comment

cca8df6

Optimize code

2a51286

tavplubix mentioned this pull request Jun 23, 2020

Merging #10679 #11896

Merged

tavplubix added a commit that referenced this pull request Jun 24, 2020

Merge pull request #11896 from ClickHouse/merging_drop_replica

2401bc9

Merging #10679

tavplubix merged commit 2a51286 into ClickHouse:master Jun 24, 2020

Conversation

sundy-li commented May 5, 2020 • edited by alexey-milovidov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filimonov commented May 6, 2020

Uh oh!

filimonov commented May 6, 2020

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!

sundy-li commented May 8, 2020

Uh oh!

alexey-milovidov commented May 11, 2020

Uh oh!

alexey-milovidov commented May 11, 2020

Uh oh!

tavplubix commented May 13, 2020

Uh oh!

sundy-li commented May 14, 2020

Uh oh!

amudong commented May 26, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cw9 commented Jun 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sundy-li commented May 5, 2020 •

edited by alexey-milovidov

Loading

amudong commented May 26, 2020 •

edited

Loading