Multiple endpoints for connection. #310

MikhailBurdukov · 2023-05-25T09:03:23Z

Specify multiple hosts for connection, to provide fault tolerance.

.SetHosts({ getEnvOrDefault("CLICKHOUSE_HOST",      "asasdasd"), 
                            getEnvOrDefault("CLICKHOUSE_HOST",      "localhost"),
                            getEnvOrDefault("CLICKHOUSE_HOST",      "noalocalhost"), 
                            getEnvOrDefault("CLICKHOUSE_HOST",      "localhost"), 
                           })

The endpoint is selecting according to EndpointsIterationAlgorithm, rn only RoundRobin approach is implemented.

Ref #139

clickhouse/base/endpoints_iterator.cpp

clickhouse/base/endpoints_iterator.h

clickhouse/client.cpp

clickhouse/base/socket.cpp

aalexfvk · 2023-05-25T13:22:03Z

clickhouse/client.cpp

@@ -194,30 +207,55 @@ class Client::Impl {
    std::unique_ptr<InputStream> input_;
    std::unique_ptr<OutputStream> output_;
    std::unique_ptr<SocketBase> socket_;
+    std::shared_ptr<EndpointsIteratorBase> endpoints_iterator;


Do we really need to store iterator here ? Or we can create it as needed. It looks pretty lightweight

For the Round Robin algo, we have to keep the same instance of the object, because otherwise current_index will be reset to zero every time the object is created.

I agree with Vasily and believe endpoints iteration-related code could be simplified. Having a base class overridden by a specific algorithm is a good general solution, but in case only one algorithm is implemented, I think it's better to have a dead simple RoundRobinEndpointsIterator endpoints_iterator without inheritance and smart pointers.

If you still want a general solution, consider using std::unique_ptr as we seem to have only one user of this field.

IMO general solution with base class looks pretty good here, fixed shared_ptr to unique_ptr by now. @Enmk what do you think?

clickhouse/client.h

ut/client_ut.cpp

clickhouse/base/endpoints_iterator.cpp

myrrc · 2023-06-05T12:45:14Z

clickhouse/client.cpp

@@ -194,30 +207,55 @@ class Client::Impl {
    std::unique_ptr<InputStream> input_;
    std::unique_ptr<OutputStream> output_;
    std::unique_ptr<SocketBase> socket_;
+    std::shared_ptr<EndpointsIteratorBase> endpoints_iterator;


I agree with Vasily and believe endpoints iteration-related code could be simplified. Having a base class overridden by a specific algorithm is a good general solution, but in case only one algorithm is implemented, I think it's better to have a dead simple RoundRobinEndpointsIterator endpoints_iterator without inheritance and smart pointers.

If you still want a general solution, consider using std::unique_ptr as we seem to have only one user of this field.

Enmk

Requires changes, please take a look at previous attempt, since this new one already repeats some of the errors and comments from the old one:
#139

MikhailBurdukov · 2023-06-06T12:39:51Z

In the last commit, a behavior was added that allows users to reset the connection to a connected endpoint, which is described here. I also checked manually that the client is trying to reconnect to the same endpoint using options_.send_retries, but I don't know how to implement this in the test, because as far as I understand, there are no tests that allow you to manipulate the connection or the `ClickHouse-server' directly. Does anyone have some ideas how to test such situation?

Enmk · 2023-06-08T12:33:04Z

clickhouse/client.h

@@ -44,6 +44,19 @@ enum class CompressionMethod {
    LZ4     =  1,
 };

+struct Endpoint {
+    std::string host;
+    unsigned int port = 9000;


Suggested change

unsigned int port = 9000;

uint16_t port = 9000;

Just to make sure that nobody tires to connect to port >65535

So maybe we should change here too?
https://github.com/ClickHouse/clickhouse-cpp/blob/dbbdefcbec5aff8f087bc0979f12ba26d0a06d5f/clickhouse/client.h#LL59C54-L59C54

Yep, I think this is a good idea, even though this alters public interface

Enmk · 2023-06-20T15:22:58Z

clickhouse/base/socket.h

@@ -135,7 +136,7 @@ class NonSecureSocketFactory : public SocketFactory {
 public:
    ~NonSecureSocketFactory() override;

-    std::unique_ptr<SocketBase> connect(const ClientOptions& opts) override;
+    std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const std::string& host, const std::string& port) override;


Suggested change

std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const std::string& host, const std::string& port) override;

std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const Endpoint & endpoint) override;

Also please modify the call site.

Enmk · 2023-06-20T15:24:01Z

clickhouse/base/socket.h

@@ -88,7 +89,7 @@ class SocketFactory {

    // TODO: move connection-related options to ConnectionOptions structure.

-    virtual std::unique_ptr<SocketBase> connect(const ClientOptions& opts) = 0;
+    virtual std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const std::string& host, const std::string& port) = 0;


Suggested change

virtual std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const std::string& host, const std::string& port) = 0;

std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const Endpoint & endpoint) = 0;

Enmk · 2023-06-20T15:25:42Z

clickhouse/base/endpoints_iterator.h

+   virtual void Next() = 0;
+   // Get the address of current endpoint. 
+   virtual std::string GetHostAddr() const = 0;
+
+   // Get the port of current endpoint.
+   virtual uint16_t GetPort() const = 0;
+
+   // Reset iterations.
+   virtual void ResetIterations() = 0; 
+   virtual bool NextIsExist() const = 0;


Suggested change

virtual void Next() = 0;

// Get the address of current endpoint.

virtual std::string GetHostAddr() const = 0;

// Get the port of current endpoint.

virtual uint16_t GetPort() const = 0;

// Reset iterations.

virtual void ResetIterations() = 0;

virtual bool NextIsExist() const = 0;

virtual Endpoint Next() = 0;

Cleaner and much simpler API. We don't need to reset iteration or to check if there are more endpoints left. Consider EndpointsIteratorBase to be an endless generator of endpoints.

If some new iteration strategy pops up later, that would require changes, we can update the interface accordingly, since it is not part of public API.

Yes, but ResetIterations is needed in order not to create an infinite loop for endpoint choosing.
Otherwise we have to check it thought opts.endpoints.size(),which doesn't look like a good solution. Any ideas how to do so?

Enmk · 2023-06-20T15:35:12Z

clickhouse/client.cpp

+    if (opts.host.empty())
+        return opts;
+
+    Endpoint endpoint_single({opts.host, opts.port});


Suggested change

Endpoint endpoint_single({opts.host, opts.port});

Endpoint default_endpoint({opts.host, opts.port});

Enmk · 2023-06-20T15:37:00Z

clickhouse/client.cpp

+        return opts;
+
+    Endpoint endpoint_single({opts.host, opts.port});
+    if (std::find(opts.endpoints.begin(), opts.endpoints.end(), endpoint_single) == std::end(opts.endpoints)) {


IMO it is best to make default_endpoint first endpoint unconditionally, so it is simple, easy and predictable

Enmk · 2023-06-20T15:38:45Z

clickhouse/client.cpp

@@ -64,8 +64,12 @@ struct ClientInfo {
 };

 std::ostream& operator<<(std::ostream& os, const ClientOptions& opt) {
-    os << "Client(" << opt.user << '@' << opt.host << ":" << opt.port
-       << " ping_before_query:" << opt.ping_before_query
+    os << "Client(";


That ignores opt.host and opt.port completely, and this operator<< might be used on unmodified ClientOptions (by tests or users), making output confusing.

Enmk · 2023-06-20T17:25:04Z

clickhouse/client.cpp

@@ -111,6 +115,10 @@ std::unique_ptr<SocketFactory> GetSocketFactory(const ClientOptions& opts) {
        return std::make_unique<NonSecureSocketFactory>();
 }

+std::unique_ptr<EndpointsIteratorBase> GetEndpointsIterator(const std::vector<Endpoint>& endpoints) {


Suggested change

std::unique_ptr<EndpointsIteratorBase> GetEndpointsIterator(const std::vector<Endpoint>& endpoints) {

std::unique_ptr<EndpointsIteratorBase> GetEndpointsIterator(const ClientOptions& options) {

MikhailBurdukov · 2023-06-27T12:20:09Z

Discussed in direct messages:

Connection format: N times to all hosts. Example with 3 endpoints and N = 2 : 1- 2 -3 -1 - 2 -3.
Iterator must have only one method: virtual Endpoint Next() = 0; and it is an endless generator of endpoints.
The logic of connection control in Client Impl. Number of connection attempts = endpoints.size() * send_retry
If the host has become unavailable, we first try to recreate the connection with it, if it did not work, then we try to find a new one.

All this is done in the last commit

MikhailBurdukov added 10 commits May 24, 2023 08:00

Draft

551ae09

Rename <host> to <endpoint>

0897577

Fix

f964bdb

Equal sizes exeption

1c6aa2f

fixes

fc27ec6

Added unit tests

615d120

minor changes

289343e

Added knob

f0bf631

comments'

6833281

Remove unused

1f52a68

aalexfvk reviewed May 25, 2023

View reviewed changes

MikhailBurdukov added 2 commits May 25, 2023 14:08

conservations resolving

8255db6

fix

a6795bb

MikhailBurdukov marked this pull request as ready for review May 25, 2023 14:21

MikhailBurdukov closed this Jun 5, 2023

MikhailBurdukov reopened this Jun 5, 2023

Enmk reviewed Jun 5, 2023

View reviewed changes

clickhouse/client.h Outdated Show resolved Hide resolved

Enmk reviewed Jun 5, 2023

View reviewed changes

clickhouse/client.h Outdated Show resolved Hide resolved

Enmk reviewed Jun 5, 2023

View reviewed changes

ut/client_ut.cpp Outdated Show resolved Hide resolved

Enmk reviewed Jun 5, 2023

View reviewed changes

clickhouse/base/endpoints_iterator.cpp Outdated Show resolved Hide resolved

myrrc suggested changes Jun 5, 2023

View reviewed changes

Enmk suggested changes Jun 5, 2023

View reviewed changes

Reviews fixes

cc40bbc

Enmk reviewed Jun 8, 2023

View reviewed changes

Enmk mentioned this pull request Jun 14, 2023

new feature needed : suport coroutine for co_yield c++20 #305

Open

Fix tests and reviews changes

2525038

Enmk reviewed Jun 20, 2023

View reviewed changes

Review changes

87804a0

Enmk merged commit de6f56a into ClickHouse:master Jul 10, 2023

p-linnane mentioned this pull request Sep 27, 2023

clickhouse-cpp 2.5.0 Homebrew/homebrew-core#146329

Merged

Enmk mentioned this pull request Sep 30, 2023

Segfault caused by outputstream being null. #335

Closed

	std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const std::string& host, const std::string& port) override;
	std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const Endpoint & endpoint) override;

	virtual std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const std::string& host, const std::string& port) = 0;
	std::unique_ptr<SocketBase> connect(const ClientOptions& opts, const Endpoint & endpoint) = 0;

	Endpoint endpoint_single({opts.host, opts.port});
	Endpoint default_endpoint({opts.host, opts.port});

	std::unique_ptr<EndpointsIteratorBase> GetEndpointsIterator(const std::vector<Endpoint>& endpoints) {
	std::unique_ptr<EndpointsIteratorBase> GetEndpointsIterator(const ClientOptions& options) {

Multiple endpoints for connection. #310

Multiple endpoints for connection. #310

Uh oh!

Conversation

MikhailBurdukov commented May 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aalexfvk May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enmk left a comment

Choose a reason for hiding this comment

Uh oh!

MikhailBurdukov commented Jun 6, 2023

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!

Enmk Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enmk Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MikhailBurdukov commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aalexfvk May 25, 2023 •

edited

Loading

Enmk Jun 20, 2023 •

edited

Loading

Enmk Jun 20, 2023 •

edited

Loading

MikhailBurdukov commented Jun 27, 2023 •

edited

Loading