-
Notifications
You must be signed in to change notification settings - Fork 35
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
[restatectl] Introduces replicated-loglet top-level command #2231
Conversation
d4c56ea
to
f063825
Compare
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.
LGTM. +1 for merging.
MetadataContainer enum was ~450 bytes and it's being used often. It also forces users to clone input which is not great. The PR contains mostly mechanical changes.
This introduces a lookup table to find replicated loglets by id in Log metadata. This also promotes replicated loglet as a core feature in restate-types since it'll become the default soon. In future follow-up we can explore deserializing the loglet params prematurely (or use typed structure instead of string) and maintain reference-counted container to avoid memory bloat. In this PR, we treat replicated loglet as a special case to speed up implementation and since this will be the de-facto loglet provider anyway.
Currently this only implements a very basic `info`. Note that the command is built merely as an example, it's UI/UX and behaviour will change by follow-ups. Pass by stringified log_id (`<log_id>_<segment>`) ``` cargo run -q --bin restatectl -- replicated-loglet info 1_0 ✘ 1 Log Configuration (v1) Loglet Referenced in: - LogId=1, Segment=0 Loglet Parameters: Loglet Id: 1_0 (4294967296) Sequencer: N1:1 Replication: {node: 2} Node Set: [N3, N2, N1] ``` Or raw loglet id (`<loglet_id>`) ``` cargo run -q --bin restatectl -- replicated-loglet info 1_0 ✘ 1 Log Configuration (v1) Loglet Referenced in: - LogId=1, Segment=0 Loglet Parameters: Loglet Id: 1_0 (4294967296) Sequencer: N1:1 Replication: {node: 2} Node Set: [N3, N2, N1] ```
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.
Apologies for the slow turnaround, just a minor callout on info vs. describe - we can update other places to info
if we want to standardize on this sub-command rather.
|
||
#[derive(Run, Parser, Collect, Clone, Debug)] | ||
#[cling(run = "get_info")] | ||
pub struct InfoOpts { |
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.
No strong opinion her but we've used the verb "describe" for this type of command elsewhere - we can solve with aliases and the internal struct doesn't need to be consistent, but it may be nice for users to see the same set of sub-commands everywhere.
Currently this only implements a very basic
info
. Note that the command is built merely as an example, it's UI/UX and behaviour will change by follow-ups.Pass by stringified log_id (
<log_id>_<segment>
)Or raw loglet id (
<loglet_id>
)Stack created with Sapling. Best reviewed with ReviewStack.