Add list of hosts with ports for setting connection #1

df530 · 2021-11-29T15:30:55Z

SetHost can take std::vector of ClientOptions::HostPort structures, that contains host name and optionally port number. If HostPort.port is nullopt, then port from SetPort (or default port if SetPort wasn't called) is used.

When ResetConnection is called, it tries to connect to the first host. If connection failed, then it tries to connect to the next host in list, while list doesn't end up. If list ends up, ResetConnection throws exception of last host.

PolyProgrammist · 2021-12-20T18:50:56Z

clickhouse/client.cpp

+            if (!Handshake()) {
+                throw std::runtime_error("fail to connect to " + host_port.host);
+            }
+        } catch (...) {


Better to catch specific error

Why is it better? We can't predict what type of object will be thrown. Yea, in this function we throw only runtime_error, but we don't exactly know what could be thrown from functions, that are used in ResetConnection.

Moreover, in RetryGuard previous developers don't catch specific error -- they also use .... It is partly confirms hypothesis, that any object can be thrown from subfunctions of ResetConnetction.

If we can't predict the type of exception, we don't know the code. But we should do.
It is better because when you read the code, you understand what you throw.

You can catch runtime_error and other exceptions and then use the ...

PolyProgrammist · 2021-12-20T18:53:26Z

tests/simple/main.cpp

@@ -510,6 +510,67 @@ int main() {
                            .SetCompressionMethod(CompressionMethod::LZ4));
            RunTests(client);
        }
+
+        {
+            std::cout << "test 1" << std::endl;


Please give names to tests

Oo, I deleted this line before review, but fogot to commit it. I think I would better to leave tests without names, because previous tests don't have names.

PolyProgrammist · 2021-12-20T18:54:48Z

tests/simple/main.cpp

@@ -481,7 +481,7 @@ static void RunTests(Client& client) {
    ArrayExample(client);
    CancelableExample(client);
    DateExample(client);
-    DateTime64Example(client);
+//    DateTime64Example(client);


Please don't delete tests

I also forgot to commit uncommented line.

I had commented it because this example always fails there.

Ok, you are right, but I am not sure it's your problem, so you can keep it unchanged for now

PolyProgrammist · 2021-12-20T18:57:05Z

tests/simple/main.cpp

+                                           ClientOptions::HostPort("1127.91.2.2"), // wrong host
+                                           ClientOptions::HostPort("notlocalwronghost"), // wrong host
+                                           ClientOptions::HostPort("another_notlocalwronghost"), // wrong host
+                                           ClientOptions::HostPort("localhost", 9000),


Can you assert somehow that you have connected to exactly this host? And please add some more hosts after this one

Hosts added.

By previous version I couldn't assert connected host. But I add method getConnectedHost to Client and use it for assert.

PolyProgrammist · 2021-12-20T19:08:18Z

clickhouse/client.h

+    std::string host = std::string();
+    inline ClientOptions& SetHost(const std::string& value) {
+        hosts_ports.emplace_back(value, std::nullopt);
+        host = hosts_ports.back().host;


Ok, so you keep host and HostPort. So host is duplicated. Can it be kept just one time?

Yes, it can be kept just one time, I made relevant changes.

PolyProgrammist · 2021-12-20T19:09:27Z

clickhouse/client.cpp

-       << " ping_before_query:" << opt.ping_before_query
+    os << "Client(" << opt.user << '@' << "{ ";
+    for (const ClientOptions::HostPort& hp : opt.hosts_ports) {
+        os << hp.host << ":" << opt.port << ",";


Try to make output so that if you have just one host it is the same as before your changes

Done and also fixed other errors (like forgotten }, etc)

PolyProgrammist · 2021-12-20T19:16:55Z

clickhouse/client.h

    /// Hostname of the server.
-    DECLARE_FIELD(host, std::string, SetHost, std::string());
+    std::string host = std::string();


You need to remove it so not to duplicate. If you don't remove it somehow, you need to declare this field with DECLARE_FIELD

I can't remove host because it is a public field.
I returned declaration by DECLARE_FIELD

Well, ok, leave it as is for now. But it would be great to write it in the pull request description that the host field is "duplicated"

Why this host is duplicated? Now host in host field isn't stored in hosts_ports.

df530 added 5 commits November 22, 2021 18:45

Add setting host list and round robin prototype

0d87218

Add timeout and tests (not debugged)

0b74858

Some changes, not fixed test, but improve action

d713ce7

Fix tests

0f2da33

Delete extra changes

25b7fdc

df530 changed the title ~~List of hosts with ports and triend connect to all~~ Add list of hosts with ports for setting connection Dec 13, 2021

PolyProgrammist reviewed Dec 20, 2021

View reviewed changes

df530 added 6 commits December 21, 2021 18:40

Fix review comments

662a27d

Divided catch (...) block

53affd2

Add descriptioin of feature to README

fabac4d

Fix font size

edf5421

merge new feature with master

563b7b6

Delete unused operator== overload

13294d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add list of hosts with ports for setting connection #1

Add list of hosts with ports for setting connection #1

df530 commented Nov 29, 2021 •

edited

Loading

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 28, 2021

PolyProgrammist Dec 28, 2021

df530 Dec 31, 2021

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 28, 2021

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 20, 2021

df530 Dec 21, 2021

PolyProgrammist Dec 28, 2021

df530 Dec 31, 2021

PolyProgrammist Jan 10, 2022

Add list of hosts with ports for setting connection #1

Are you sure you want to change the base?

Add list of hosts with ports for setting connection #1

Conversation

df530 commented Nov 29, 2021 • 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

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

df530 commented Nov 29, 2021 •

edited

Loading