[MDB-29707] Metatdata cleaner with system drop replica #241

MikhailBurdukov · 2024-10-04T10:09:34Z

A little performance test:

500 replicated tables + 0 ReplicatedDB
    old 5 runs: 
        median:  0m44.959s
        list : [2m56.825s, 2m56.635s, 0m44.959s, 0m41.463s, 0m44.481s]
    new 5 runs:  
        median:  0m9.965s
        list: [0m6.865s, 0m9.965s, 0m10.014s, 0m6.701s, 0m10.085s]


500 replicated tables + 200 Replicated databases
    old 5 runs: 
        median:  3m39.932s
        list : [0m50.593s, 0m52.412s, 3m39.697s, 3m40.830s, 3m39.932s]
    new 5 runs:  
        median:  0m8.550s
        list: [0m13.685s, 0m13.335s, 0m8.550s, 0m8.320s, 0m8.627s]

gen script:

REPLICA="non_exists"
DB='tests_db'

clickhouse client --query "ATTACH DATABASE $DB"
clickhouse client --query "DROP DATABASE $DB"

clickhouse client --query "CREATE DATABASE $DB"


for i in {1..500}
do
   QUERY="CREATE TABLE $DB.t$i (a int) ENGINE=ReplicatedMergeTree('/clickhouse/tables/$DB/t$i','$REPLICA') ORDER BY a"

   echo "$QUERY"
   clickhouse client --query "$QUERY"
done

clickhouse client --query "DETACH DATABASE $DB"


for i in {1..200}
do
        DB_REPL="repl$i"
        clickhouse client --query "ATTACH DATABASE $DB_REPL"
        clickhouse client --query "DROP DATABASE IF EXISTS $DB_REPL"
        QUERY="CREATE DATABASE $DB_REPL ENGINE = Replicated('/clickhouse/databases/$DB_REPL', 'shard1', '$REPLICA')"
        echo $QUERY
        clickhouse client --query "$QUERY"
        clickhouse client --query "DETACH DATABASE $DB_REPL"
done

MikhailBurdukov · 2024-10-04T10:10:05Z

ch_tools/common/config.py

@@ -37,6 +37,7 @@
        "unfreeze_timeout": 10 * 60,
        "restart_replica_timeout": 10 * 60,
        "restore_replica_timeout": 10 * 60,
+        "drop_replica_timeout": 10 * 60,


10 min is enough? or set it to 1 hour?

I think its ok. We will adjust it if necessary

MikhailBurdukov · 2024-10-04T12:40:26Z

System drop database replica was announced in 23.1. So we can't use it for 22.8.
That version already deprecated, but we have small number of clusters with that version.

Should we disable replicated database cleaning for 22.8? It is pretty new feature, maybe we don't even have users who uses them.
Or keep backoff to the old behaviour?

WDYT @Alex-Burmak @aalexfvk ?

aalexfvk · 2024-10-04T14:39:58Z

ch_tools/chadmin/internal/zookeeper.py

+        """
+
+        if block_until_finised_tasks:
+            self._queue_active[0][1].wait()


self._queue_active can be empty

aalexfvk · 2024-10-04T14:41:38Z

ch_tools/chadmin/internal/zookeeper.py

+        self._max_active_tasks = max_parrallel_tasks
+
+        self._queue_pending: Deque[str] = deque()
+        self._queue_active: Deque[Tuple[str, IAsyncResult]] = deque(


Let's make dataclass for the tuple to get rid of naked indices like [0][1]

aalexfvk · 2024-10-04T14:56:41Z

ch_tools/chadmin/internal/zookeeper.py

+
+        if not self._exists_tasks_to_do():
+            return None
+


It looks like we can add waiting for the first ready result here and remove with_block and block_until_finished_tasks parameters from everywhere

Yes, we can. But this will lead to unnecessary waiting every time we call the queue update.

What if we remove waiting from _update_active_queue at all and insert it here ?

moved implementation with wait to the separate function

aalexfvk · 2024-10-04T15:17:09Z

ch_tools/chadmin/internal/zookeeper.py

-            replica_path = os.path.join(path, "replicas")
-            if not zk.exists(replica_path):
+        for replicated_object in replicated_objects:
+            # Actually rn in the ch (10.24) there are no secure way to determine that node is the root of replicated table.


Suggested change

# Actually rn in the ch (10.24) there are no secure way to determine that node is the root of replicated table.

# Actually there is no a reliable way to determine that node is the root of replicated table in the CH (24.10)

aalexfvk · 2024-10-04T15:21:19Z

ch_tools/chadmin/internal/zookeeper.py

-            if not zk.exists(replica_path):
+        for replicated_object in replicated_objects:
+            # Actually rn in the ch (10.24) there are no secure way to determine that node is the root of replicated table.
+            # So we are accuming that if object not database then it is table.


Suggested change

# So we are accuming that if object not database then it is table.

# So we are assuming that if object is not a database then it is a table.

aalexfvk · 2024-10-04T15:28:19Z

ch_tools/chadmin/internal/zookeeper.py

+                is_database = bool(
+                    zk.get(replicated_object.path)[0] == REPLICATED_DATABASE_MARKER
+                )


Why is casting to bool needed here ?

aalexfvk · 2024-10-04T15:31:14Z

ch_tools/chadmin/internal/zookeeper.py

+                        if replicated_object.path not in databases_to_cleanup:
+                            databases_to_cleanup[replicated_object.path] = []


How about using defaultdict for the databases_to_cleanup ?

aalexfvk · 2024-10-04T15:36:28Z

ch_tools/common/config.py

@@ -37,6 +37,7 @@
        "unfreeze_timeout": 10 * 60,
        "restart_replica_timeout": 10 * 60,
        "restore_replica_timeout": 10 * 60,
+        "drop_replica_timeout": 10 * 60,


I think its ok. We will adjust it if necessary

This reverts commit 6502ec3.

#244) This reverts commit 6502ec3.

Metatdata cleaner with system drop replica

de6a77d

MikhailBurdukov commented Oct 4, 2024

View reviewed changes

Add replicated databases in conf

7a89ac0

MikhailBurdukov requested review from Alex-Burmak, aalexfvk and kirillgarbar October 4, 2024 12:28

MikhailBurdukov changed the title ~~Metatdata cleaner with system drop replica~~ [MDB-29707] Metatdata cleaner with system drop replica Oct 4, 2024

Fix 22.8

7108e01

aalexfvk reviewed Oct 4, 2024

View reviewed changes

MikhailBurdukov added 2 commits October 7, 2024 07:36

Review fixes

fc66e22

More fixes

6490956

aalexfvk approved these changes Oct 8, 2024

View reviewed changes

aalexfvk merged commit 6502ec3 into main Oct 8, 2024
26 checks passed

aalexfvk deleted the metadata_cleaner branch October 8, 2024 07:27

MikhailBurdukov added a commit that referenced this pull request Oct 14, 2024

Revert "[MDB-29707] Metatdata cleaner with system drop replica (#241)"

d420cc0

This reverts commit 6502ec3.

MikhailBurdukov mentioned this pull request Oct 14, 2024

Revert "[MDB-29707] Metatdata cleaner with system drop replica" #244

Merged

aalexfvk pushed a commit that referenced this pull request Oct 14, 2024

Revert "[MDB-29707] Metatdata cleaner with system drop replica (#241)" (

40bda1f

#244) This reverts commit 6502ec3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDB-29707] Metatdata cleaner with system drop replica #241

[MDB-29707] Metatdata cleaner with system drop replica #241

MikhailBurdukov commented Oct 4, 2024 •

edited

Loading

MikhailBurdukov Oct 4, 2024

aalexfvk Oct 4, 2024

MikhailBurdukov commented Oct 4, 2024 •

edited

Loading

aalexfvk Oct 4, 2024

aalexfvk Oct 4, 2024

aalexfvk Oct 4, 2024

MikhailBurdukov Oct 7, 2024

aalexfvk Oct 7, 2024

MikhailBurdukov Oct 7, 2024

aalexfvk Oct 4, 2024

aalexfvk Oct 4, 2024

aalexfvk Oct 4, 2024

aalexfvk Oct 4, 2024

aalexfvk Oct 4, 2024

	# Actually rn in the ch (10.24) there are no secure way to determine that node is the root of replicated table.
	# Actually there is no a reliable way to determine that node is the root of replicated table in the CH (24.10)

	# So we are accuming that if object not database then it is table.
	# So we are assuming that if object is not a database then it is a table.

		if replicated_object.path not in databases_to_cleanup:
		databases_to_cleanup[replicated_object.path] = []

[MDB-29707] Metatdata cleaner with system drop replica #241

[MDB-29707] Metatdata cleaner with system drop replica #241

Conversation

MikhailBurdukov commented Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikhailBurdukov commented Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikhailBurdukov commented Oct 4, 2024 •

edited

Loading

MikhailBurdukov commented Oct 4, 2024 •

edited

Loading