Change ZooKeeper path for zero-copy locks for shared data #32061

ianton-ru · 2021-12-01T13:23:46Z

Changelog category (leave one):

Backward Incompatible Change

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Change ZooKeeper path for zero-copy marks for shared data.
Fix for remove marks in ZooKeeper for renamed parts.

Detailed description / Documentation draft:
Before locks for shared data was in table node in zookeeper.
As result after removing part in table or entry table was no more info about shared use, and UNFREEZE/DROP DETACHED removes S3-objects required in other replicas (in other freezed backup for example).
Now this info keeps outside table node.
This change breaks backward compatibility.
Upgrade script converts records in ZooKeeper.
Fix for remove "lost" locks leaved after part renames.

…DROP DETACHED PART

src/Storages/MergeTree/IMergeTreeDataPart.cpp

alesapin · 2021-12-10T11:38:59Z

src/Storages/StorageReplicatedMergeTree.cpp

@@ -7515,4 +7560,193 @@ bool StorageReplicatedMergeTree::createEmptyPartInsteadOfLost(zkutil::ZooKeeperP
    return true;
 }

+
+void StorageReplicatedMergeTree::convertZeroCopySchema()


No, we shouldn't implement compatibility inside clickhouse server for non-production ready features. Let's implement a seperate tool for it.

Ok. Separate convertaion tool looks easy to make, but without downtime with a low chance to data corruption during process, when some replicas are updated, some yet not, when part created after convertation process and destroyed before it complete. Looks like this is accepted risk.

Move it in script utils/zero_copy/zero_copy_schema_converter.py

alesapin · 2021-12-20T12:21:13Z

src/Storages/MergeTree/MergeTreeData.cpp

@@ -5203,6 +5292,75 @@ PartitionCommandsResultInfo MergeTreeData::unfreezeAll(
    return unfreezePartitionsByMatcher([] (const String &) { return true; }, backup_name, local_context);
 }

+bool MergeTreeData::removeSharedDetachedPart(DiskPtr disk, const String & path, const String & part_name)
+{
+    bool keep_shared = false;


Why we have it as a separate variable? It doesn't change anywhere. removeSharedRecursive takes it by value.

At least const bool keep_shared

You are right. This variable is useless after refactoring.
Removed (and wholly method renamed to 'removeDetachedPart' and logic moved to 'StorageReplicatedMergeTree').

alesapin · 2021-12-20T12:28:00Z

src/Storages/MergeTree/MergeTreeData.cpp

+        FreezeMetaData meta;
+        if (meta.load(disk, path))
+            return removeSharedDetachedPart(disk, path, part_name, meta.zookeeper_name, meta.replica_name,
+                    "", meta.is_replicated);


Why zookeeper_path empty?????

Freezed part with existing FreezeMetaData can be created only in updated version of ClickHouse. In this case it cannot have zero-copy record on old place.

alesapin · 2021-12-20T12:30:18Z

src/Storages/MergeTree/MergeTreeData.cpp

+        zkutil::ZooKeeperPtr zookeeper;
+        if (zookeeper_name == default_zookeeper_name)
+        {
+            zookeeper = getContext()->getZooKeeper();


It turns our not perfect abstractions into trash:

grep 'ZooKeeper' src/Storages/MergeTree/MergeTreeData.{h,cpp} | wc -l 0

Logic moved to StorageReplicatedMergeTree.

alesapin · 2021-12-20T12:31:11Z

src/Storages/MergeTree/MergeTreeData.cpp

+    return keep_shared;
+}
+
+bool MergeTreeData::removeSharedDetachedPart(DiskPtr disk, const String & path, const String & part_name,


Why this "shared" code is here?

When we unfreeze freezed part for removed table then we have no info in ClickHouse is this part replicated or not. There are only files on disk and may be lock in ZooKeeper, but none MergeTreeData object for this non existing table.
I see no other way except call one universal method with support both variants inside.

May be this code should be in some kind of "MergeTreeGarbageCollector" class. Is it better?

But now none logic to unfreeze entry backup, parts unfreezed only for existing tables, so possible to move code to StorageReplicatedMergeTree.

Logic moved to StorageReplicatedMergeTree.

src/Storages/MergeTree/MergeTreeData.h

alesapin · 2021-12-20T12:32:19Z

src/Storages/MergeTree/MergeTreeData.h

    /// Fetch part only if some replica has it on shared storage like S3
    /// Overridden in StorageReplicatedMergeTree
    virtual bool tryToFetchIfShared(const IMergeTreeDataPart &, const DiskPtr &, const String &) { return false; }

+    virtual String getZooKeeperName() const { return ""; }


No, MergeTreeData doesn't know anything about zookeeper.

Moved to StorageReplicatedMergeTree.

alesapin · 2021-12-24T13:54:49Z

src/Storages/StorageReplicatedMergeTree.cpp

+            }
+            else if (code != Coordination::Error::ZOK)
+            {
+                throw zkutil::KeeperException(code, zookeeper_table_id_path);


Maybe it's better to retry if it's concurrent process?

tryCreate can return only one of ZOK, ZNONODE, ZNODEEXISTS, ZNOCHILDRENFOREPHEMERALS
ZNONODE and ZNOCHILDRENFOREPHEMERALS mean that something wrong with table root node. Concurrent creation can produce only ZNODEEXISTS, in this case one replica wins, other replicas get this error and should read id from zookeeper.

alesapin · 2021-12-24T13:57:27Z

src/Storages/MergeTree/IMergeTreeDataPart.h

    String getUniqueId() const;

+    /// Return hardlink count for part.
+    /// Required for keep data on remote FS when part has shadow copies.
+    UInt32 getRefCount() const;


Need to clarify that it's not some general-purpose refcount. The name should be something like getNumberOfReferenecesForZeroCopyReplication.

Renamed to getNumberOfRefereneces

alesapin · 2021-12-24T13:58:05Z

src/Storages/MergeTree/MergeTreeData.cpp

-        localBackup(part->volume->getDisk(), src_part_path, backup_part_path);
+        localBackup(disk, src_part_path, backup_part_path);
+
+        freezeMetaData(disk, part, backup_part_path);


Add a comment -- "Store metadata for replicated tables and do nothing for ordinary merge tree"

alesapin · 2021-12-24T13:58:43Z

src/Storages/StorageReplicatedMergeTree.cpp

+}
+
+
+void StorageReplicatedMergeTree::freezeMetaData(DiskPtr disk, DataPartPtr, String backup_part_path) const


Better to name it createAndStoreFreezeMetadata.

alesapin · 2021-12-24T13:59:30Z

src/Storages/StorageReplicatedMergeTree.h

@@ -263,6 +274,10 @@ class StorageReplicatedMergeTree final : public shared_ptr_helper<StorageReplica

    bool createEmptyPartInsteadOfLost(zkutil::ZooKeeperPtr zookeeper, const String & lost_part_name);

+    String getZooKeeperName() const { return zookeeper_name; }
+
+    virtual String getTableUniqID() const override;


Don't need virtual here. Also need a comment about purpose.

alesapin · 2021-12-24T14:12:03Z

src/Storages/StorageReplicatedMergeTree.cpp

+    zkutil::ZooKeeperPtr zookeeper;
+    if (detached_zookeeper_name == default_zookeeper_name)
+    {
+        zookeeper = getContext()->getZooKeeper();


Why we cannot just use table's zookeeper with getZooKeeper(). ZooKeeper may change for some reason?

I remove it now. Later we will need this for unfreeze parts of removed tables.

alesapin · 2021-12-24T14:12:46Z

src/Storages/StorageReplicatedMergeTree.cpp

+        fs::path checksums = fs::path(path) / "checksums.txt";
+        if (disk->exists(checksums))
+        {
+            auto ref_count = disk->getRefCount(checksums);


we have getRefCount in IMergeTreeDataPart? Why we cannot use it here?

At this moment we haven't existing part object, only detached files on disk.

alesapin · 2021-12-24T14:14:36Z

src/Storages/StorageReplicatedMergeTree.cpp

+            if (ref_count == 0)
+            {
+                String id = disk->getUniqueId(checksums);
+                keep_shared = !StorageReplicatedMergeTree::unlockSharedDataById(id, table_uuid, part_name,


How is it possible, when ref_count == 0 and unlockSharedDataById still says keep_shared?

ref_count is local counter for freezed/detached/etc copies of metadata on this replica.
unlockSharedDataById remove lock for this replica in ZooKeeper and check if any locks of other replicas exist.

alesapin · 2021-12-24T14:28:18Z

src/Storages/StorageReplicatedMergeTree.cpp

+
+    String zero_copy = fmt::format("zero_copy_{}", toString(disk_type));
+
+    String new_path = fs::path(settings.remote_fs_zero_copy_zookeeper_path.toString()) / zero_copy / table_uuid / part_name;


So now we store all zero copy data on a single path in ZK?

It splitted by table_id.
I can't use something like table zookeeper_path, because in this case after drop table and recreate with same zookeeper path will be collisions between old and new part locks.

alesapin · 2021-12-24T14:29:47Z

src/Storages/MergeTree/MergeTreeData.cpp

@@ -2568,6 +2572,8 @@ bool MergeTreeData::renameTempPartAndReplace(
            out_covered_parts->emplace_back(std::move(covered_part));
    }

+    part->cleanupOldName(old_part_name);


Add something in changelog about it.

…-15474

alesapin

Ok, fix is important so let's merge it.

alesapin · 2022-01-07T10:12:07Z

@Mergifyio update

mergify · 2022-01-07T10:12:26Z

update

✅ Branch has been successfully updated

ianton-ru added 5 commits November 18, 2021 21:14

Add test for freeze/unfreeze with S3 zero-copy

827fa51

Zero-copy: move shared mark outside table node in ZooKeeper

0f9038e

Fix wait for freeze in tests

d409ab0

Fix Zero-Copy replication lost locks, fix remove used remote data in …

80ab73c

…DROP DETACHED PART

Add tests for new zero-copy schema

c8fe1dc

robot-clickhouse added the pr-backward-incompatible Pull request with backwards incompatible changes label Dec 1, 2021

ianton-ru changed the title ~~Mdb 15474~~ Change ZooKeeper path for zero-copy marks for shared data Dec 1, 2021

ianton-ru added 3 commits December 1, 2021 19:03

Merge master

e0a16a4

Fix types

0e685c1

Fix tests

98bae1b

alesapin reviewed Dec 13, 2021

View reviewed changes

ianton-ru added 3 commits December 20, 2021 16:19

Use table UUID in zero-copy shared label in ZooKeeper

f0b9a43

Remove zero-copy version converter

c724b07

Add zero-copy version converted script

f390111

alesapin reviewed Dec 20, 2021

View reviewed changes

ianton-ru and others added 3 commits December 21, 2021 17:27

Merge master

0c0bf66

Move logic for replicated part to StorageReplicatedMergeTree class

33cbfc8

Merge branch 'master' into MDB-15474

e6fd4bf

ianton-ru force-pushed the MDB-15474 branch from b56b20b to ba5f182 Compare December 21, 2021 15:02

Fix typos

e88b97d

ianton-ru force-pushed the MDB-15474 branch from ba5f182 to e88b97d Compare December 21, 2021 16:57

alesapin self-assigned this Dec 22, 2021

alesapin changed the title ~~Change ZooKeeper path for zero-copy marks for shared data~~ Change ZooKeeper path for zero-copy locks for shared data Dec 24, 2021

alesapin reviewed Dec 24, 2021

View reviewed changes

ianton-ru and others added 6 commits December 27, 2021 19:27

Fixes by code review responces

0465aef

Fix debug build

2d87f0a

Merge branch 'master' into ianton-ru-MDB-15474

7a3c874

Remove method from IStorage

8b331cd

Merge branch 'master' into ianton-ru-MDB-15474

cbdba89

Merge branch 'master' into MDB-15474

92cb451

alesapin added 2 commits December 30, 2021 12:57

Tiny improvements

91e1ac4

Merge remote-tracking branch 'ianton-ru/MDB-15474' into ianton-ru-MDB…

b426cc4

…-15474

alesapin approved these changes Dec 30, 2021

View reviewed changes

Merge branch 'master' into MDB-15474

6fcd5a7

alesapin merged commit 733ed7c into ClickHouse:master Jan 8, 2022

		}


		void StorageReplicatedMergeTree::freezeMetaData(DiskPtr disk, DataPartPtr, String backup_part_path) const


		String zero_copy = fmt::format("zero_copy_{}", toString(disk_type));

		String new_path = fs::path(settings.remote_fs_zero_copy_zookeeper_path.toString()) / zero_copy / table_uuid / part_name;

Change ZooKeeper path for zero-copy locks for shared data #32061

Change ZooKeeper path for zero-copy locks for shared data #32061

Conversation

ianton-ru commented Dec 1, 2021 • edited by alesapin

Choose a reason for hiding this comment

ianton-ru Dec 14, 2021 • edited

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

Choose a reason for hiding this comment

ianton-ru Dec 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianton-ru Dec 21, 2021 • edited

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

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

alesapin left a comment

Choose a reason for hiding this comment

alesapin commented Jan 7, 2022

mergify bot commented Jan 7, 2022

✅ Branch has been successfully updated

ianton-ru commented Dec 1, 2021 •

edited by alesapin

ianton-ru Dec 14, 2021 •

edited

ianton-ru Dec 21, 2021 •

edited

ianton-ru Dec 21, 2021 •

edited