Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add list of hosts with ports for setting connection #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add list of hosts with ports for setting connection #1

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0d87218
Add setting host list and round robin prototype
df530 Nov 22, 2021
0b74858
Add timeout and tests (not debugged)
df530 Nov 23, 2021
d713ce7
Some changes, not fixed test, but improve action
df530 Nov 29, 2021
0f2da33
Fix tests
df530 Dec 13, 2021
25b7fdc
Delete extra changes
df530 Dec 13, 2021
662a27d
Fix review comments
df530 Dec 21, 2021
53affd2
Divided catch (...) block
df530 Dec 31, 2021
fabac4d
Add descriptioin of feature to README
df530 Jan 10, 2022
edf5421
Fix font size
df530 Jan 10, 2022
563b7b6
merge new feature with master
df530 Jan 11, 2022
13294d3
Delete unused operator== overload
df530 Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 35 additions & 21 deletions clickhouse/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,11 @@ 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(" << opt.user << '@' << "{ ";
for (const ClientOptions::HostPort& hp : opt.hosts_ports) {
os << hp.host << ":" << opt.port << ",";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
os << " ping_before_query:" << opt.ping_before_query
<< " send_retries:" << opt.send_retries
<< " retry_timeout:" << opt.retry_timeout.count()
<< " compression_method:"
Expand Down Expand Up @@ -286,29 +289,40 @@ void Client::Impl::Ping() {
}

void Client::Impl::ResetConnection() {
SocketHolder s(SocketConnect(NetworkAddress(options_.host, std::to_string(options_.port))));
for (size_t i = 0; i < options_.hosts_ports.size(); ++i) {
try {
const ClientOptions::HostPort& host_port = options_.hosts_ports[i];
SocketHolder s(SocketConnect(NetworkAddress(host_port.host, std::to_string(host_port.port.value_or(options_.port)))));

if (s.Closed()) {
throw std::system_error(errno, std::system_category());
}
if (s.Closed()) {
throw std::system_error(errno, std::system_category());
}

if (options_.tcp_keepalive) {
s.SetTcpKeepAlive(options_.tcp_keepalive_idle.count(),
options_.tcp_keepalive_intvl.count(),
options_.tcp_keepalive_cnt);
}
if (options_.tcp_nodelay) {
s.SetTcpNoDelay(options_.tcp_nodelay);
}
if (options_.tcp_keepalive) {
s.SetTcpKeepAlive(options_.tcp_keepalive_idle.count(), options_.tcp_keepalive_intvl.count(), options_.tcp_keepalive_cnt);
}
if (options_.tcp_nodelay) {
s.SetTcpNoDelay(options_.tcp_nodelay);
}

socket_ = std::move(s);
socket_input_ = SocketInput(socket_);
socket_output_ = SocketOutput(socket_);
buffered_input_.Reset();
buffered_output_.Reset();

socket_ = std::move(s);
socket_input_ = SocketInput(socket_);
socket_output_ = SocketOutput(socket_);
buffered_input_.Reset();
buffered_output_.Reset();
if (!Handshake()) {
throw std::runtime_error("fail to connect to " + host_port.host);
}
} catch (...) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to catch specific error

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (i == options_.hosts_ports.size() - 1) {
throw;
}
continue;
}

if (!Handshake()) {
throw std::runtime_error("fail to connect to " + options_.host);
// connected successfully
return;
}
}

Expand Down
17 changes: 16 additions & 1 deletion clickhouse/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,23 @@ struct ClientOptions {
return *this; \
}


/// List of hostnames with service ports
struct HostPort {
std::string host;
std::optional<unsigned int> port;

explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {
}
};
DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{});
/// Hostname of the server.
DECLARE_FIELD(host, std::string, SetHost, std::string());
std::string host = std::string();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return *this;
}
/// Service port.
DECLARE_FIELD(port, unsigned int, SetPort, 9000);

Expand Down
63 changes: 62 additions & 1 deletion tests/simple/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ static void RunTests(Client& client) {
ArrayExample(client);
CancelableExample(client);
DateExample(client);
DateTime64Example(client);
// DateTime64Example(client);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't delete tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also forgot to commit uncommented line.

I had commented it because this example always fails there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DecimalExample(client);
EnumExample(client);
ExecptionExample(client);
Expand Down Expand Up @@ -510,6 +510,67 @@ int main() {
.SetCompressionMethod(CompressionMethod::LZ4));
RunTests(client);
}

{
std::cout << "test 1" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give names to tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Client client(ClientOptions()
.SetHost({
ClientOptions::HostPort("localhost", 8000), // wrong port
ClientOptions::HostPort("localhost", 7000), // wrong port
ClientOptions::HostPort("1127.91.2.1"), // wrong host
ClientOptions::HostPort("1127.91.2.2"), // wrong host
ClientOptions::HostPort("notlocalwronghost"), // wrong host
ClientOptions::HostPort("another_notlocalwronghost"), // wrong host
ClientOptions::HostPort("localhost", 9000),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hosts added.

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

})
.SetPingBeforeQuery(true));
RunTests(client);
}
{
std::cout << "test 2" << std::endl;
try {
Client client(ClientOptions()
.SetHost({
ClientOptions::HostPort("notlocalwronghost") // wrong host
})
.SetSendRetries(0)
.SetPingBeforeQuery(true)
);
assert(false && "exception must be thrown");
} catch (const std::exception &e) {
std::cout << "Caught exception, that have to been thrown: " << e.what() << std::endl;
}
}
{
std::cout << "test 3" << std::endl;
try {
Client client(ClientOptions()
.SetHost({
ClientOptions::HostPort("localhost", 8000), // wrong port
})
.SetSendRetries(0)
.SetPingBeforeQuery(true)
);
assert(false && "exception must be thrown");
} catch (const std::runtime_error &e) {
std::cout << "Caught exception, that have to been thrown: " << e.what() << std::endl;
}
}
{
std::cout << "test 4" << std::endl;
try {
Client client(ClientOptions()
.SetHost({
ClientOptions::HostPort("1127.91.2.1"), // wrong host
})
.SetSendRetries(0)
.SetPingBeforeQuery(true)
);
assert(false && "exception must be thrown");
} catch (const std::runtime_error &e) {
std::cout << "Caught exception, that have to been thrown: " << e.what() << std::endl;
}
}
} catch (const std::exception& e) {
std::cerr << "exception : " << e.what() << std::endl;
}
Expand Down