[reconfigurator] initialize clickhouse cluster db schema #7306
Conversation
| )); | ||
| let admin_url = format!("http://{admin_addr}"); | ||
| let log = opctx.log.new(slog::o!("admin_url" => admin_url.clone())); | ||
| let client = ClickhouseSingleClient::new(&admin_url, log.clone()); |
There was a problem hiding this comment.
Should this be ClickhouseServerClient instead of ClickhouseSingleClient? Right now they happen to have compatible APIs for this one method (.init_db), but we probably don't want to depend on that.
There was a problem hiding this comment.
whoops! good catch. Thanks!
There was a problem hiding this comment.
Looks great @karencfv! Thank you so much for doing this.
Can you please test adding a node to see what happens with the schema? I don't believe it will work without initializing it specifically.
| @@ -160,6 +160,38 @@ pub(crate) async fn deploy_nodes( | |||
| "Successfully deployed all clickhouse server and keeper configs" | |||
| ); | |||
|
|
|||
| // We only need to initialise the database schema into one of the ClickHouse replica | |||
There was a problem hiding this comment.
I believe this is only true for the initial cluster. If you add a new node, it will not get initialized automatically. Since the operation is idempotent I would go ahead and initialize all the servers.
There was a problem hiding this comment.
If you add a new node, it will not get initialized automatically.
ugh, that's a bummer. yep! will change then
| } | ||
|
|
||
| enum ClickhouseAdminKeeperImpl {} | ||
|
|
||
| impl ClickhouseAdminKeeperApi for ClickhouseAdminKeeperImpl { | ||
| type Context = Arc<ServerContext>; | ||
| type Context = Arc<KeeperServerContext>; |
There was a problem hiding this comment.
Good call on the rename here ;)
| "initializing replicated ClickHouse cluster to version {OXIMETER_VERSION}" | ||
| ); | ||
| ctx.oximeter_client() | ||
| .initialize_db_with_version(true, OXIMETER_VERSION) |
There was a problem hiding this comment.
nit: APIs with bool parameters make it hard to figure out what is being passed in at the callsite. I usually create a temp variable to name the flag then pass that in. I'd suggest that here with something like:
let replicated = true;
ctx.oximeter_client().initialize_db_with_version(replicated, OXIMETER_VERSION);
This commit allows reconfigurator to initialise the oximeter DB schema on a replicated cluster via clickhouse admin.
Tested by deploying Omicron locally and setting/enabling the clickhouse policy to "both"
Closes: #7060