feat: Adds tdbg migration script to power migrations via visibility#10495
feat: Adds tdbg migration script to power migrations via visibility#10495davidporter-id-au wants to merge 8 commits into
Conversation
|
chaptersix
left a comment
There was a problem hiding this comment.
Review of the tdbg bulk migration changes. Overall looks solid -- nice dry-run default and structured logging. A few issues around flag validation and the default visibility query direction.
| if err != nil { | ||
| return fmt.Errorf("unable to open output log %q: %w", logPath, err) | ||
| } | ||
| defer func() { _ = logFile.Close() }() |
There was a problem hiding this comment.
If ListWorkflowExecutions fails mid-pagination, workers may have already migrated some schedules, but summary.print is never called -- the user has no idea what already happened. Move summary.print(c, execute) before the listErr check so partial progress is always reported.
| if len(nextPageToken) == 0 { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
--output-log is silently ignored in stdin mode -- the migrateSummary is created without wiring up logEnc from FlagOutputLog. Either handle it here too, or reject the flag combination.
There was a problem hiding this comment.
I think this now addressed with some validation at the top of the function
| var listErr error | ||
| var nextPageToken []byte | ||
| for { | ||
| ctx, cancel := newContext(c) |
There was a problem hiding this comment.
--workers is also silently ignored in stdin mode (processes sequentially). Consider either applying the worker pool here too, or validating that --workers isn't set when using stdin.
There was a problem hiding this comment.
now returns an error
There was a problem hiding this comment.
Also, not sure if this comment was from an earlier line, it seems misaligned? no big deal either way
| &cli.BoolFlag{ | ||
| Name: FlagExecute, | ||
| Usage: "Perform the migration. Without this flag, --from-visibility and stdin modes only print what they would do (dry-run)", | ||
| }, |
There was a problem hiding this comment.
--query is accepted without --from-visibility and silently ignored (both in --schedule-id and stdin modes). Worth validating the combination up front.
There was a problem hiding this comment.
I think tihs is addressed now
| os.Stdin = f | ||
| defer func() { | ||
| os.Stdin = orig | ||
| _ = f.Close() |
There was a problem hiding this comment.
nit: withStdin mutates os.Stdin which is process-global. Worth a comment that these tests must not use t.Parallel().
There was a problem hiding this comment.
Yeah, this one annoyed me. I was considering a larger refactor to fix but it got out of hand. Imho it'd be better to pass the os.ReadWriter interface down rather than just reaching in to Os.Stdin like this
would be great to add this to the help text |
What changed?
Adds a cli set of flags to the tdbg schedule migrate subcommand, to fetch from visibility (or stdin) and perform a migration.
Using Visibility
Using stdin:
Why?
Temporal is moving to a V2/Chasm based scheduler. Migrating schedules manually is easier when done via visibility, so this is a hopefully more ergonomic set of commands for such a migration:
How did you test it?
Potential risks
Not likely to be a lot relating to the tool itself, the migration path probably needs a bit more testing through.