[reconfigurator] Endpoint to retrieve keeper cluster membership #6758
Conversation
|
|
||
| /// The configuration of the clickhouse keeper raft cluster returned from a | ||
| /// single keeper node | ||
| /// | ||
| /// Each keeper is asked for its known raft configuration via `clickhouse-admin` | ||
| /// dropshot servers running in `ClickhouseKeeper` zones. state. We include the | ||
| /// leader committed log index known to the current keeper node (whether or not | ||
| /// it is the leader) to determine which configuration is newest. | ||
| #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] | ||
| pub struct ClickhouseKeeperClusterMembership { | ||
| pub queried_keeper: KeeperId, | ||
| pub leader_committed_log_index: u64, | ||
| pub raft_config: BTreeSet<KeeperId>, | ||
| } |
There was a problem hiding this comment.
I had to move this to avoid a circular dependency
| raft_output.keeper_servers.iter().map(|s| s.server_id).collect(); | ||
|
|
||
| Ok(ClickhouseKeeperClusterMembership { | ||
| queried_keeper: conf_output.server_id, |
There was a problem hiding this comment.
@andrewjstone I do remember you mentioned we could retrieve the queried_keeper from somewhere else.
I kinda like the idea of the keeper reporting its own server ID though. This way in case we called the wrong address for some reason or whatever, we will always be 100% sure we have the ID of the keeper we queried, not the keeper we think we queried. WDYT?
There was a problem hiding this comment.
This works fine, but I think it will be unnecessary in the near future.
What I was talking about is that when we write the keeper and server configs, we should also store an extra file that contains the generation number from the KeeperConfigurableSettings and ServerConfigurableSettings of the last written config. We would cache that generation number in the clickhouse-admin server and load it from the file on startup. We need to do this as an optimization so that we don't constantly overwrite the clickhouse config files and force them to be reloaded when nothing has changed.
I was thinking that instead of just saving the generation number we could save the entire ServerConfigurableSettings and KeeperConfigurableSettings and cache those as well. Doing that would allow us to just look in memory of the clickhouse-admin server for the keeper-id. And since we use these settings to write the keeper config files, the values would always be the same.
| /// single keeper node | ||
| /// | ||
| /// Each keeper is asked for its known raft configuration via `clickhouse-admin` | ||
| /// dropshot servers running in `ClickhouseKeeper` zones. state. We include the |
There was a problem hiding this comment.
I know you just moved this struct, but there's a typo I made that you may as well cleanup while you're here.
| /// dropshot servers running in `ClickhouseKeeper` zones. state. We include the | |
| /// dropshot servers running in `ClickhouseKeeper` zones. We include the |
| raft_config.insert(clickhouse_admin_types::KeeperId(i)); | ||
| } | ||
|
|
||
| let expected_keeper_cluster_membership = |
There was a problem hiding this comment.
Personally, I wouldn't even bother defining the whole struct just to pull out the 2 fields we care about. I'd just compare what we get back to the values directly.
| raft_output.keeper_servers.iter().map(|s| s.server_id).collect(); | ||
|
|
||
| Ok(ClickhouseKeeperClusterMembership { | ||
| queried_keeper: conf_output.server_id, |
There was a problem hiding this comment.
This works fine, but I think it will be unnecessary in the near future.
What I was talking about is that when we write the keeper and server configs, we should also store an extra file that contains the generation number from the KeeperConfigurableSettings and ServerConfigurableSettings of the last written config. We would cache that generation number in the clickhouse-admin server and load it from the file on startup. We need to do this as an optimization so that we don't constantly overwrite the clickhouse config files and force them to be reloaded when nothing has changed.
I was thinking that instead of just saving the generation number we could save the entire ServerConfigurableSettings and KeeperConfigurableSettings and cache those as well. Doing that would allow us to just look in memory of the clickhouse-admin server for the keeper-id. And since we use these settings to write the keeper config files, the values would always be the same.
There was a problem hiding this comment.
Thanks for the review @andrewjstone 🙇♀️ I'll merge this PR, and as we talked about, we'll make the small changes in a follow up PR
Overview
This commit implements a new clickhouse-admin endpoint to retrieve information from the keeper cluster. It returns
ClickhouseKeeperClusterMembershipManual testing