-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
Split clickhouse admin server #6837
Conversation
Also, split the clients and remove the dependency of clickhouse-admin-api from nexus.
I'm pretty sure I have to fix some omicron-pkg and smf stuff here as well. I'll do that shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The new SMF definition looks very similar to the existing keeper one so I'll presume it's okay. Everything else looks pretty mechanical. Thanks!
|
||
/// We separate the admin interface for the keeper and server APIs because they |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First paragraph of doc comments should be short (a lint for this is coming in Rust 1.83.) Could you just add something simple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip! Done in 0a2e28b
@@ -95,3 +77,28 @@ pub trait ClickhouseAdminApi { | |||
rqctx: RequestContext<Self::Context>, | |||
) -> Result<HttpResponseOk<ClickhouseKeeperClusterMembership>, HttpError>; | |||
} | |||
|
|||
/// We separate the admin interface for the keeper and server APIs because they |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0a2e28b
@@ -17,7 +17,7 @@ | |||
</dependency> | |||
|
|||
<exec_method type='method' name='start' | |||
exec='ctrun -l child -o noorphan,regent /opt/oxide/clickhouse-admin/bin/clickhouse-admin run -c /var/svc/manifest/site/clickhouse-admin/config.toml -a %{config/http_address} -l %{config/ch_address} -b %{config/ch_binary} &' | |||
exec='ctrun -l child -o noorphan,regent /opt/oxide/clickhouse-admin-keeper/bin/clickhouse-admin-keeper run -c /var/svc/manifest/site/clickhouse-admin-keeper/config.toml -a %{config/http_address} -l %{config/ch_address} -b %{config/ch_binary} &' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I ask you to use the long version of command arguments here? ^_^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0a2e28b
</dependency> | ||
|
||
<exec_method type='method' name='start' | ||
exec='ctrun -l child -o noorphan,regent /opt/oxide/clickhouse-admin-server/bin/clickhouse-admin-server run -c /var/svc/manifest/site/clickhouse-admin-server/config.toml -a %{config/http_address} -l %{config/ch_address} -b %{config/ch_binary} &' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0a2e28b
Thanks for the great review @sunshowers! |
Fixes #6779
Fixes #6829