Add list of hosts reading #4
Conversation
| @@ -29,6 +29,7 @@ class Client : public ClientBase | |||
| const std::vector<Arguments> & external_tables_arguments) override; | |||
| void processConfig() override; | |||
|
|
|||
| std::vector<String> hosts{}; | |||
There was a problem hiding this comment.
Where is a single host keeped? You need to find it and keep vector of hosts instead of one host
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well so you can add it to ClientBase as it is done for query-files
| @@ -512,6 +512,7 @@ catch (...) | |||
|
|
|||
| void Client::connect() | |||
| { | |||
| config().setString("host", hosts[0]); | |||
There was a problem hiding this comment.
This line shouldn't be here as soon as you remove field hosts from Client class
There was a problem hiding this comment.
It is a crutch, which I will move to initialization (more you can see in conversation below #4 (comment))
There was a problem hiding this comment.
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
| @@ -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") | |||
There was a problem hiding this comment.
Please add more detailed description. For example, like in interleave-queries-file option
There was a problem hiding this comment.
Ok, I will make it when add parsing hosts with port (to call clickhouse client --host localhost:9000)
There was a problem hiding this comment.
You can add it now and when you will change for port, you can also modify description
| @@ -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>>(); | |||
There was a problem hiding this comment.
Seems like you need to set config here, but not in the beginning of Client::connect
There was a problem hiding this comment.
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.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
The param
--hostcan receive some hosts. In the future client will use it to choose host for connection. Choise will be depend on availability and workload of hosts.Detailed description / Documentation draft:
...