Replication of user-defined SQL functions using ZooKeeper #46085

aalexfvk · 2023-02-06T16:21:24Z

Added support of replication of user-defined functions across nodes in cluster by means of storing in ZooKeeper.
Enabled by specifying a key user_defined_zookeeper_path in the config.xml.
Added support of backup/restore replicated user-defined functions via BACKUP/RESTORE TABLE system.functions ON CLUSTER for consistent behaviour.

This PR is largely based on this PR #42145.

Changelog category (leave one):

New Feature

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

Add replication of user-defined SQL functions using ZooKeeper

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

myrrc · 2023-02-28T17:10:24Z

src/Backups/BackupCoordinationRemote.cpp

+        String path2 = path + "/" + escaped_loader_zk_path;
+
+        if (zk->exists(path2 + "/functions"))
+        {


Maybe it would be easier to use designated if with temporary variables instead of path2 and path3?

Suggested change

String path2 = path + "/" + escaped_loader_zk_path;

if (zk->exists(path2 + "/functions"))

{

if (String func_path = fmt::format("{}/{}/functions", path, escaped_loader_path); zk->exists(func_path)) {

Refactored a bit

myrrc · 2023-02-28T17:20:44Z

src/Backups/BackupCoordinationReplicatedSQLObjects.h

+    void addDirectory(const String & loader_zk_path, UserDefinedSQLObjectType object_type, const String & host_id, const String & dir_path);
+
+    /// Returns all added paths to directories if `host_id` is a host chosen to store user-defined SQL objects.
+    Strings getDirectories(const String & loader_zk_path, UserDefinedSQLObjectType object_type, const String & host_id) const;
+
+private:
+    using ZkPathAndObjectType = std::pair<String, UserDefinedSQLObjectType>;


I'd recommend changing ZkPathAndObjectType to a structure ZkPathAndObject. Using std::pair and std::tuple is usually error-prone. You can still use uniform initialization {zk_path, object_type} or (unlike std::pair) you can use designated initializers which account for better readability. The word Type in my opinion does not add any information (as using keyword denotes a type) and should be removed.

I'd recommend changing addDirectory and getDirectories signatures to take ZkPathAndObject (the same generally applies to src/Backups/IBackupCoordination.h).

As for 1:

First, I want to point that it is consistent with code for access_storage.

The ...Type here is a part of ...ObjectType and not the whole alias. It denotes second element of pair. Just Object would be incorrect.

std::pair is not always error-prone. For example, like here it is used as a key for map which is quite a common practice. Because it has predefined comparisons operators (<=> actually). And it is not accessed via it first, second which is really ugly.

As for 2:

Frankly, i don't see need to introduce another indirection level for parameters. At least for now.

myrrc · 2023-02-28T17:25:43Z

src/Backups/RestoreCoordinationLocal.h

+    bool acquireReplicatedSQLObjects(const String & loader_zk_path, UserDefinedSQLObjectType object_type) override;
+


Another small question about naming: are there any other replicated SQL objects that are processed, or it's only about UDF?. In the latter case would you consider renaming functions to smth. like acquireReplicatedUDF?

There are no others at the moment. Only UDF. I think it is better to leave it as it is. The function can start working with new type (with a small modification, if there will be any.

myrrc · 2023-02-28T17:34:42Z

src/Common/ZooKeeper/ZooKeeperCachingGetter.h

+namespace zkutil
+{
+
+class ZooKeeperCachingGetter : boost::noncopyable


It's up to ClickHouse maintainers to decide, but I tend to believe this feature (which in turn could be done with CacheBase) should be separated to another PR.

CacheBase functionality is definitely not needed here. Perhaps the word caching confused you. What it means here is that it returns previously saved session if it is not expired or recreated the new one otherwise.

In general, this class could not been extracted but i found this useful.

myrrc · 2023-02-28T17:36:22Z

src/Functions/UserDefined/UserDefinedSQLObjectType.h

+const char * toString(UserDefinedSQLObjectType type);
+


You don't need to write such functions as ClickHouse has enum reflection
and you can simply write fmt::format({}, my_enum)

Did it for consistency with the rest of the code, for example.
Is it accepted in the project not to write any more toString ?

I think it's better to adopt no-more-boilerplate practices, and I'm currently working actively to remove all usages of toString(), but it's up to you as for now I guess

Removed it. And used enum reflection capabilities.

aalexfvk · 2023-03-01T09:24:54Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.cpp

+UserDefinedSQLObjectsLoaderFromZooKeeper::UserDefinedSQLObjectsLoaderFromZooKeeper(
+    const ContextPtr & global_context_, const String & zookeeper_path_)
+    : global_context{global_context_}
+    , zookeeper_getter{[global_context_]() { return global_context_->getZooKeeper(); }}


Umm... lambda captures parameter by copy in this case and stores its own instance of the shared_ptr. But if wrote [&global_context] then yes lambda would store the reference.

myrrc · 2023-02-28T17:46:11Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.h

+
+    ThreadFromGlobalPool watching_thread;
+    std::atomic<bool> watching_flag = false;
+    std::shared_ptr<ConcurrentBoundedQueue<std::pair<UserDefinedSQLObjectType, String>>> watch_queue;


Please consider reusing ZkPathAndObject from https://github.com/ClickHouse/ClickHouse/pull/46085/files#diff-16b6bfd00f65d958566847206d85dbb1b9ae301b7bec9df2616878ba83a9dbf0

String in watch_queue is not a zk path, it's the name of an object. Apparently we need a comment here about that.

Added self-documenting alias for pair.

myrrc · 2023-02-28T17:52:25Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.cpp

+
+UserDefinedSQLObjectsLoaderFromZooKeeper::~UserDefinedSQLObjectsLoaderFromZooKeeper()
+{
+    SCOPE_EXIT_SAFE(stopWatchingThread());


SCOPE_EXIT macro is used to simplify control flow in case of early exits. This destructor does not have any complex control flow statements, maybe you should replace to

Suggested change

SCOPE_EXIT_SAFE(stopWatchingThread());

stopWatchingThread();

What is used here is primarily that it does try/catch with logging and gets rid of a little boilerplate code.
The question here is rather whether an abnormal fail is allowed here, in case of an exception when the thread is stopped. So far, I've decided it's better to log it

myrrc · 2023-02-28T17:57:05Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.cpp

+    size_t num_attempts = 10;
+    while (true)


Suggested change

size_t num_attempts = 10;

while (true)

for(size_t i = 0; i < num_attempts; ++i)

I'm not sure your version is much better, note the exception generation below after the last failed attempt.

myrrc · 2023-02-28T18:03:25Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.cpp

+    auto object_list_watcher = [watch_queue = watch_queue, object_type](const Coordination::WatchResponse &)
+    { [[maybe_unused]] bool push_result = watch_queue->emplace(object_type, ""); };


You can just ignore the emplace result or use the [[maybe_unused]] attribute without creating a temporary variable.

emplace is marked by [[nodiscard]] attribute. As for [[maybe_unused]], it can be applicable only to declarations (as variable in this case), but not function calling.

push_result is not a very clear name. Let's call it inserted, and let's check it with chassert

I agree. Did it

aalexfvk · 2023-03-01T09:24:54Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.cpp

+UserDefinedSQLObjectsLoaderFromZooKeeper::UserDefinedSQLObjectsLoaderFromZooKeeper(
+    const ContextPtr & global_context_, const String & zookeeper_path_)
+    : global_context{global_context_}
+    , zookeeper_getter{[global_context_]() { return global_context_->getZooKeeper(); }}


Umm... lambda captures parameter by copy in this case and stores its own instance of the shared_ptr. But if wrote [&global_context] then yes lambda would store the reference.

vitlibar · 2023-03-15T21:49:22Z

src/Common/ConcurrentBoundedQueue.h

@@ -109,7 +109,7 @@ class ConcurrentBoundedQueue
    template <typename... Args>
    [[nodiscard]] bool emplace(Args &&... args)
    {
-        return emplaceImpl(std::nullopt /* timeout in milliseconds */, std::forward<Args...>(args...));


What's the difference?

The initial version was incorrect, actually. It forwarded all pack at once but forwarding each argument is needed, otherwise value categories wouldn't be preserved.

Yes, the former variant std::forward<Args...>(args...) is specialized to smth like this std::forward<Args_1, Args_2, ...>(args_1, args_2, ...)); that is incorrect because std::forward accepts only one argument.
And thanks to @myrrc for the link to the documentation

vitlibar · 2023-03-15T21:55:39Z

src/Common/ZooKeeper/ZooKeeperCachingGetter.h

+
+    /// Returns the ZooKeeper session and the flag whether it was taken from the cache(false) or opened new(true),
+    /// because the session has expired or the cache was empty
+    std::pair<zkutil::ZooKeeperPtr, bool> getZooKeeper();


enum instead of bool here could look better

Reworked to use nested enum

vitlibar · 2023-03-15T21:57:48Z

src/Functions/UserDefined/UserDefinedSQLObjectType.h

@@ -9,4 +9,6 @@ enum class UserDefinedSQLObjectType
    Function
 };

+const char * toString(UserDefinedSQLObjectType type);


Is this function called anywhere?

vitlibar · 2023-03-15T22:15:09Z

src/Functions/UserDefined/UserDefinedSQLObjectsLoaderFromZooKeeper.cpp

+{
+    const auto object_watcher = [watch_queue = watch_queue, object_type, object_name](const Coordination::WatchResponse & response)
+    {
+        if (response.type == Coordination::Event::CHANGED) [[maybe_unused]]


maybe_unused is specified at the wrong line

Corrected this

robot-ch-test-poll3 added the pr-feature Pull request with new product feature label Feb 6, 2023

aalexfvk force-pushed the alexfvk/store_udf_in_zookeeper branch 2 times, most recently from 61bcfd7 to 722fb6f Compare February 6, 2023 16:35

qoega added the can be tested Allows running workflows for external contributors label Feb 6, 2023

aalexfvk force-pushed the alexfvk/store_udf_in_zookeeper branch 2 times, most recently from 80e6813 to 302784e Compare February 7, 2023 08:03

vitlibar self-assigned this Feb 7, 2023

aalexfvk force-pushed the alexfvk/store_udf_in_zookeeper branch from 302784e to a48a7ac Compare February 27, 2023 16:07

myrrc suggested changes Feb 28, 2023

View reviewed changes

aalexfvk commented Mar 1, 2023

View reviewed changes

vitlibar reviewed Mar 15, 2023

View reviewed changes

aalexfvk force-pushed the alexfvk/store_udf_in_zookeeper branch from a48a7ac to a874b9d Compare March 17, 2023 10:52

aalexfvk added 5 commits March 17, 2023 13:56

Add classes for replication UDF to ZooKeeper

f84aa2f

Tests for replication UDF to ZooKeeper

1321f87

Add classes for backup/restore UDF

690d835

Tests for replicated system.functions

cf6ca10

[rev: 1] Fix review remarks

886b530

aalexfvk force-pushed the alexfvk/store_udf_in_zookeeper branch from a874b9d to 886b530 Compare March 17, 2023 10:57

vitlibar approved these changes Mar 20, 2023

View reviewed changes

vitlibar merged commit 198409e into ClickHouse:master Mar 20, 2023

vitlibar mentioned this pull request Mar 23, 2023

Improve replicated user defined functions #47919

Merged

UnamedRus mentioned this pull request Apr 12, 2023

Shared metadata storage #48620

Closed

UnamedRus mentioned this pull request Jun 15, 2023

UDF functions from system.functions table is not replicated when starting a new node #51030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replication of user-defined SQL functions using ZooKeeper #46085

Replication of user-defined SQL functions using ZooKeeper #46085

aalexfvk commented Feb 6, 2023 •

edited

Loading

myrrc Feb 28, 2023

aalexfvk Mar 17, 2023

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023 •

edited

Loading

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023 •

edited

Loading

myrrc Mar 1, 2023

aalexfvk Mar 17, 2023

aalexfvk Mar 1, 2023

myrrc Feb 28, 2023

vitlibar Mar 15, 2023

aalexfvk Mar 17, 2023

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023

myrrc Feb 28, 2023

aalexfvk Mar 1, 2023 •

edited

Loading

vitlibar Mar 15, 2023

aalexfvk Mar 17, 2023

aalexfvk Mar 1, 2023

vitlibar Mar 15, 2023

myrrc Mar 16, 2023

aalexfvk Mar 17, 2023

vitlibar Mar 15, 2023

aalexfvk Mar 17, 2023 •

edited

Loading

vitlibar Mar 15, 2023

aalexfvk Mar 17, 2023

vitlibar Mar 15, 2023

aalexfvk Mar 17, 2023

		bool acquireReplicatedSQLObjects(const String & loader_zk_path, UserDefinedSQLObjectType object_type) override;

	size_t num_attempts = 10;
	while (true)
	for(size_t i = 0; i < num_attempts; ++i)

		auto object_list_watcher = [watch_queue = watch_queue, object_type](const Coordination::WatchResponse &)
		{ [[maybe_unused]] bool push_result = watch_queue->emplace(object_type, ""); };

Replication of user-defined SQL functions using ZooKeeper #46085

Replication of user-defined SQL functions using ZooKeeper #46085

Conversation

aalexfvk commented Feb 6, 2023 • edited Loading

Changelog category (leave one):

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

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

aalexfvk Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexfvk Mar 1, 2023 • 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

Choose a reason for hiding this comment

aalexfvk Mar 1, 2023 • 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

aalexfvk Mar 17, 2023 • 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

aalexfvk commented Feb 6, 2023 •

edited

Loading

aalexfvk Mar 1, 2023 •

edited

Loading

aalexfvk Mar 1, 2023 •

edited

Loading

aalexfvk Mar 1, 2023 •

edited

Loading

aalexfvk Mar 17, 2023 •

edited

Loading