Composable protocol by yakov-olkhovskiy · Pull Request #41198 · ClickHouse/ClickHouse

yakov-olkhovskiy · 2022-09-11T22:09:19Z

Changelog category (leave one):

New Feature

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

Composable protocol configuration is added.

protocols configuration example:

    <protocols>
        <tcp>
            <type>tcp</type>
            <host>127.0.0.1</host>
            <port>9000</port>
            <description>native protocol (tcp)</description>
        </tcp>
        <tcp_secure>
            <type>tls</type>
            <impl>tcp</impl>
            <host>127.0.0.1</host>
            <port>9440</port>
            <description>secure native protocol (tcp_secure)</description>
        </tcp_secure>
        <tcp2>
            <impl>tcp</impl>
            <host>127.0.0.1</host>
            <port>9101</port>
            <description>native protocol second endpoint (tcp)</description>
        </tcp2>
        <tcp_secure2>
            <impl>tcp_secure</impl>
            <host>127.0.0.1</host>
            <port>9541</port>
            <description>secure native protocol second endpoint (tcp_secure)</description>
        </tcp_secure2>
        <mysql>
            <type>mysql</type>
            <host>127.0.0.1</host>
            <port>9004</port>
            <description>mysql</description>
        </mysql>
    </protocols>

…config

azat · 2022-09-12T07:31:30Z

programs/server/Server.cpp

+        throw Exception(ErrorCodes::INVALID_CONFIG_PARAMETER, "Protocol configuration error, unknown protocol name '{}'", type);
+    };
+
+    for (const auto & protocol : protocols)


I see that it is a WIP, but I was just passing by, and want to note about applying changes w/o server restart (Server::updateServers())

src/Server/TLSHandlerFactory.h

…lickHouse into composable-protocol

devcrafter · 2022-09-28T11:21:27Z

programs/server/Server.cpp

    bool listen_try = config.getBool("listen_try", false);
    if (!listen_try)
-        listen_try = DB::getMultipleValuesFromConfig(config, "", "listen_host").empty();
+    {


Please could you add comment what we are doing here and why?

Still have no idea - this will only affect starting interserver - lines starting at 2153 - the logic is that if config listen_try is false, make it true if no listen_host is configured (which means this node doesn't serve anything but interserver communication). And this change will preserve this logic with composable protocols.

devcrafter · 2022-09-28T11:23:45Z

src/Functions/FunctionShowCertificate.h

        throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "SSL support is disabled");
 #endif
-        return std::make_shared<FunctionShowCertificate>();
+        return std::make_shared<FunctionShowCertificate>(ctx->getQueryContext()->getClientInfo().certificate);


It's not clear if the pointers access is safe (is it guaranteed that they are not null?)

I believe it is guaranteed

tests/integration/test_composable_protocols/configs/config.xml

devcrafter · 2022-09-28T11:33:55Z

tests/integration/test_composable_protocols/configs/users.xml

+<clickhouse>
+    <profiles>
+        <default>
+            <max_memory_usage>10000000000</max_memory_usage>


Why do we need these settings?

it's just a copy-paste from some other test - does nothing

If it's not necessary, let's remove it?

src/Server/TCPProtocolStackData.h

src/Server/TCPServerConnectionFactory.h

devcrafter · 2022-09-28T11:40:26Z

src/Server/TCPProtocolStackFactory.h

+    IServer & server [[maybe_unused]];
+    Poco::Logger * log;
+    std::string conf_name;
+    std::list<TCPServerConnectionFactory::Ptr> stack;


Please replace with vector

devcrafter · 2022-09-28T12:06:38Z

programs/server/Server.cpp

+            if (config.has(prefix + "port"))
+            {


I'd use the code below to reduce unnecessary nesting

if (!config.has(...)) continue;

devcrafter · 2022-09-28T12:08:10Z

tests/integration/test_composable_protocols/test.py

@@ -0,0 +1,69 @@
+import ssl


We need to add tests with incorrect configs

it doesn't make much sense - server will just not start

Well, my PoV - we've added some checks about config correctness for layer protocol, which would be nice to test. You to decide

devcrafter · 2022-09-28T12:15:52Z

programs/server/Server.cpp

+                        throw Exception(ErrorCodes::INVALID_CONFIG_PARAMETER, "Protocol '{}' configuration contains a loop on '{}'", protocol, conf_name);
+                }
+
+                if (!stack || stack->size() == 0)


!stack - does it make sense here?

stack->size() == 0 -> stack->empty()

devcrafter · 2022-09-28T12:24:31Z

programs/server/Server.cpp

+                bool is_secure = false;
+                auto stack = std::make_unique<TCPProtocolStackFactory>(*this, conf_name);
+
+                while (true)


It'd be nice to move this logic to a function, something like buildProtocolStackFromConfig(). It'll contain create_factory() lambda as well and will not blow up createServers() method

there are too many connections in the code - dispatching it through function parameters will be quite cumbersome and will make code less clear

devcrafter · 2022-10-05T15:29:55Z

tests/integration/test_composable_protocols/configs/users.xml

+<clickhouse>
+    <profiles>
+        <default>
+            <max_memory_usage>10000000000</max_memory_usage>


If it's not necessary, let's remove it?

devcrafter · 2022-10-05T15:32:49Z

tests/integration/test_composable_protocols/test.py

@@ -0,0 +1,69 @@
+import ssl


Well, my PoV - we've added some checks about config correctness for layer protocol, which would be nice to test. You to decide

devcrafter · 2022-10-05T15:38:09Z

programs/server/Server.cpp

    http_params->setTimeout(settings.http_receive_timeout);
    http_params->setKeepAliveTimeout(keep_alive_timeout);

+    Poco::Util::AbstractConfiguration::Keys protocols;


I still would ask you to move this code to a separate method/function. The method will not have more parameters than Server::createServers, composable protocol creation code will be localized in certain method and will not blow up existing method.

src/Server/ProxyV1Handler.cpp

devcrafter · 2022-10-05T16:03:00Z

src/Server/TCPProtocolStackFactory.h

+
+    void append(TCPServerConnectionFactory::Ptr factory)
+    {
+        stack.push_back(factory);


Suggested change

stack.push_back(factory);

stack.push_back(std::move(factory));

devcrafter · 2022-10-17T14:49:49Z

It'd be nice if you could address this comment, but I don't insist. The concern is, - adding new functionality this way is a right way to make code less readable

yakov-olkhovskiy · 2022-10-17T14:57:46Z

@devcrafter already did - probably you are looking at previous version?

devcrafter · 2022-10-17T15:01:42Z

@devcrafter already did - probably you are looking at previous version?

Sorry then. Thanks 👍

yakov-olkhovskiy added 11 commits September 1, 2022 22:06

add composable factory and protocol

e7959d7

test with TLS

b6e3680

add variadic constructor

5727517

protocols configuration processing

8a7fe28

add PROXYv1 handler, add stack exchange data block, tuneup protocols …

772bf05

…config

pass section config key to a factory

d001bae

some refactoring

c3ac0c4

bugs fixed, cleanup, working state

745e759

split into diferent files

0c62b5a

cleanup, respect USE_SSL

7985f4b

cleanup

ffa7d3b

robot-ch-test-poll added the pr-feature Pull request with new product feature label Sep 11, 2022

yakov-olkhovskiy and others added 3 commits September 11, 2022 22:44

style fix

7c855c9

Merge branch 'master' into composable-protocol

0d6d9e7

maybe_unused

0d89bdb

azat reviewed Sep 12, 2022

View reviewed changes

yakov-olkhovskiy and others added 5 commits September 13, 2022 23:12

respect listen_host config param, fix updateServers and getListenTry

d550604

allow to specify key and certificate for TLS layer

8f07992

Merge branch 'master' into composable-protocol

5e3fde6

bug fix, merge fix, style fix

b939379

USE_SSL fix

910d493

yakov-olkhovskiy marked this pull request as ready for review September 15, 2022 12:51

devcrafter self-assigned this Sep 16, 2022

yakov-olkhovskiy and others added 4 commits September 18, 2022 07:11

pass session certificate for showCertificate()

c66f412

add 'default_database' endpoint config parameter

fc78af3

Merge branch 'master' into composable-protocol

2f2080f

add allowed networks for endpoint

4ccfbed

devcrafter reviewed Sep 22, 2022

View reviewed changes

src/Server/TLSHandlerFactory.h Outdated Show resolved Hide resolved

yakov-olkhovskiy and others added 2 commits September 23, 2022 10:54

remove maybe_unused

c23b1f4

add test

1407f6b

yakov-olkhovskiy and others added 2 commits September 25, 2022 16:02

Merge branch 'composable-protocol' of https://github.com/ClickHouse/C…

95b95d6

…lickHouse into composable-protocol

Automatic style fix

7a6386c

devcrafter reviewed Sep 28, 2022

View reviewed changes

tests/integration/test_composable_protocols/configs/config.xml Show resolved Hide resolved

devcrafter reviewed Sep 28, 2022

View reviewed changes

src/Server/TCPProtocolStackData.h Show resolved Hide resolved

devcrafter reviewed Sep 28, 2022

View reviewed changes

src/Server/TCPServerConnectionFactory.h Outdated Show resolved Hide resolved

devcrafter reviewed Sep 28, 2022

View reviewed changes

review suggestions

5e40f2e

yakov-olkhovskiy requested a review from devcrafter September 30, 2022 15:19

devcrafter reviewed Oct 5, 2022

View reviewed changes

yakov-olkhovskiy and others added 5 commits October 7, 2022 01:10

move some functionality to Server::buildProtocolStackFromConfig

fc1de0a

review suggestions

997fa5e

Merge branch 'master' into composable-protocol

2229087

add test for PROXYv1

6bb62d4

Automatic style fix

3bed015

alexey-milovidov mentioned this pull request Oct 8, 2022

Intern Tasks 2022/2023 #42194

Closed

yakov-olkhovskiy added 2 commits October 8, 2022 20:33

Merge branch 'master' into composable-protocol

6c8e52e

Merge branch 'master' into composable-protocol

b5808a3

yakov-olkhovskiy requested a review from devcrafter October 11, 2022 05:29

devcrafter approved these changes Oct 17, 2022

View reviewed changes

yakov-olkhovskiy merged commit 3ce1fa0 into master Oct 17, 2022

yakov-olkhovskiy deleted the composable-protocol branch October 17, 2022 15:23

	stack.push_back(factory);
	stack.push_back(std::move(factory));

Conversation

yakov-olkhovskiy commented Sep 11, 2022

Changelog category (leave one):

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

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devcrafter commented Oct 17, 2022

Uh oh!

yakov-olkhovskiy commented Oct 17, 2022

Uh oh!

devcrafter commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants