-
Notifications
You must be signed in to change notification settings - Fork 709
rpk: support cluster-uuid-override when running WCR #29153
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
rpk: support cluster-uuid-override when running WCR #29153
Conversation
| cmd.Flags().MarkDeprecated("topic-name-pattern", "Not supported") | ||
| cmd.Flags().BoolVarP(&wait, "wait", "w", false, "Wait until auto-restore is complete") | ||
| cmd.Flags().DurationVar(&pollingInterval, "polling-interval", 5*time.Second, "The status check interval (e.g. '30s', '1.5m'); ignored if --wait is not used") | ||
| cmd.Flags().StringVar(&uuidOverride, "cluster-uuid-override", "", "Cluster UUID to restore from") |
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.
Maybe something like "Explicit UUID to restore from (i.e. not latest)"?
Want to make sure this does not confuse readers into thinking they must specify it. And also would be helpful to hint at what default behavior is (if left unset).
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.
Indeed. Thanks for the suggestion!
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.
Changed it to Explicit cluster UUID to restore from; uses the latest if unset in afc60d4
| recovery process until it's finished.`, | ||
| recovery process until it's finished. | ||
| Use --cluster-uuid-override if the cluster was re-initialized after the backup, |
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.
The addition is confusing. The cluster currently running can also be considered as "re-initialized after the backup" from user point of view...
Need a more precise wording.
The command description also looks wrong. This restores more than just topics.
Maybe just link to docs instead where we can be more wordy?
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.
@r-vasquez is just linking to https://docs.redpanda.com/current/manage/disaster-recovery/whole-cluster-restore/ in the command description appropriate from rpk pov?
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.
Wdyt about this?
Start the cluster restoration process.
This command starts the process of restoring data from a failed cluster with
Tiered Storage enabled, including its metadata, onto a new cluster.
If the wait flag (--wait/-w) is set, the command will poll the status of the
recovery process until it's finished.
Use --cluster-uuid-override if you want to specify a not latest cluster UUID
to restore from. For more information, visit https://docs.redpanda.com/current/manage/disaster-recovery/whole-cluster-restore/.
cc: @Feediver1
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.
Use --cluster-uuid-override if you want to specify a not latest cluster UUID
to restore from.
How about: "Use --cluster-uuid-override if you want to specify an explicit cluster UUID
to restore from"?
If you have the context than this sentence makes perfect sense. Otherwise it is vague (but not misleading) which should make the reader follow to the docs.
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.
@daisukebe yes, it's appropriate. We tend to avoid it as it can be easily changed (the URL) while changing rpk requires a release.
If we can explain all we need in our rpk help text, good, if not, linking is better than having nothing.
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.
Sounds good to me. Thanks!
11f708d to
afc60d4
Compare
CI test resultstest results on build#78675
|
nvartolomei
left a comment
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 from core perspective 👍
|
/backport v25.3.x |
rpk cluster storage restore startnow supports--cluster-uuid-overridefor a UUID override.redpanda-data/common-go#113 is the rpadmin side of things.
redpanda-data/docs#1513 needs to be updated accordingly once this is merged.
Backports Required
Release Notes
Features
--cluster-uuid-override