Skip to content

Add list of hosts reading #4

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

Closed
wants to merge 1 commit into from
Closed

Add list of hosts reading #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions programs/client/Client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ catch (...)

void Client::connect()
{
config().setString("host", hosts[0]);
Copy link
Member

Choose a reason for hiding this comment

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

This line shouldn't be here as soon as you remove field hosts from Client class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a crutch, which I will move to initialization (more you can see in conversation below #4 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

Well, so as soon as you don't want to add hosts to config, you just don't need to set it in the beginning. You should either set it in config or don't set at all. I see you want to save hosts anywhere, but please write clean code

connection_parameters = ConnectionParameters(config());

if (is_interactive)
Expand Down Expand Up @@ -998,7 +999,7 @@ void Client::addAndCheckOptions(OptionsDescription & options_description, po::va
/// Main commandline options related to client functionality and all parameters from Settings.
options_description.main_description->add_options()
("config,c", po::value<std::string>(), "config-file path (another shorthand)")
("host,h", po::value<std::string>()->default_value("localhost"), "server host")
("host,h", po::value<std::vector<std::string>>()->multitoken()->default_value({"localhost"}, "localhost"), "list of server hosts")
Copy link
Member

Choose a reason for hiding this comment

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

Please add more detailed description. For example, like in interleave-queries-file option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will make it when add parsing hosts with port (to call clickhouse client --host 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.

You can add it now and when you will change for port, you can also modify description

("port", po::value<int>()->default_value(9000), "server port")
("secure,s", "Use TLS connection")
("user,u", po::value<std::string>()->default_value("default"), "user")
Expand Down Expand Up @@ -1120,7 +1121,7 @@ void Client::processOptions(const OptionsDescription & options_description,
if (options.count("config"))
config().setString("config-file", options["config"].as<std::string>());
if (options.count("host") && !options["host"].defaulted())
config().setString("host", options["host"].as<std::string>());
hosts = options["host"].as<std::vector<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.

Seems like you need to set config here, but not in the beginning of Client::connect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Setting config() is a crutch. My purpose is to save hosts anywhere, after that collegue can use this vector to make connection. But you are right, it would be better, if this crutch will be there, not in beginnin of connect.

Copy link
Member

Choose a reason for hiding this comment

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

#4 (comment)

if (options.count("interleave-queries-file"))
interleave_queries_files = options["interleave-queries-file"].as<std::vector<std::string>>();
if (options.count("pager"))
Expand Down
1 change: 1 addition & 0 deletions programs/client/Client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class Client : public ClientBase
const std::vector<Arguments> & external_tables_arguments) override;
void processConfig() override;

std::vector<String> hosts{};
Copy link
Member

Choose a reason for hiding this comment

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

Where is a single host keeped? You need to find it and keep vector of hosts instead of one host

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A single host is kept originally in configuration object with type Poco::Util:AbstactConfiguration, which pointer you can get from method config(). Configuration is usually setting in method Client::init, and other methods, which are called in init.
To set field in config() you shoud call config().set<String | Int | ...>(<key>, <value>). But there are setting methods only for simple types: int, string, bool, etc. You can't set key with std::vector value.

I watched, how vectors are saved in other situations (when other vector parametrs are set). I found param --query-files in class ClientBase: https://github.com/ManagedDatabases/ClickHouse/blob/managed-feature/choose-server-from-list/src/Client/ClientBase.cpp#L1536

And the values are saving in special field of class ClientBase (while other simple string or int params are saved in config()): https://github.com/ManagedDatabases/ClickHouse/blob/managed-feature/choose-server-from-list/src/Client/ClientBase.cpp#L1600

That is why I've made an similar field for hosts in Client class.

Copy link
Member

Choose a reason for hiding this comment

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

Well so you can add it to ClientBase as it is done for query-files

private:
void printChangedSettings() const;
std::vector<String> loadWarningMessages();
Expand Down
2 changes: 1 addition & 1 deletion src/Client/ClientBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1580,7 +1580,7 @@ void ClientBase::init(int argc, char ** argv)

/// Output of help message.
if (options.count("help")
|| (options.count("host") && options["host"].as<std::string>() == "elp")) /// If user writes -help instead of --help.
|| (options.count("host") && options["host"].as<std::vector<std::string>>()[0] == "elp")) /// If user writes -help instead of --help.
{
printHelpMessage(options_description);
exit(0);
Expand Down