Restructure Admin TaskList commands to operate on multiple types #6712
Conversation
natemort
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| prettyPrintJSONObject(getDeps(c).Output(), response) | ||
| return nil | ||
| } | ||
|
|
||
| fmt.Println("Task Lists for domain " + domain + ":") |
There was a problem hiding this comment.
Not for this diff, but mixing print statements with the stdout writer is :sadpanda:
| } else if c.String(FlagTaskListType) == "" { | ||
| taskListTypes = []types.TaskListType{types.TaskListTypeActivity, types.TaskListTypeDecision} | ||
| } else { | ||
| return nil, commoncli.Problem("Invalid task list type: valid types are [activity, decision]", nil) |
There was a problem hiding this comment.
| return nil, commoncli.Problem("Invalid task list type: valid types are [activity, decision]", nil) | |
| return nil, commoncli.Problem("Invalid task list type: valid types are 'activity', 'decision', or empty (both)", nil) |
| msg := fmt.Sprintf("Successfully updated %s:%s", tl.Name, tlType) | ||
| _, err = getDeps(c).Output().Write([]byte(msg)) |
There was a problem hiding this comment.
should be able to
fmt.Fprintf(getDeps(c).Output(), "Successfully updated %s:%s", tl.Name, tlType)and I think errcheck won't complain about the fprintf err. but this is just an FYI / if-convenient-simplification, no need to change imo
There was a problem hiding this comment.
That reads better, switched to that. Goland at least complains about it, so I'll add the explicit ignore.
| if len(errors) > 0 { | ||
| return commoncli.Problem("Potentially unsafe operation. Specify '--force' to proceed anyway", multierr.Combine(errors...)) | ||
| } | ||
| if !hasPollers { |
There was a problem hiding this comment.
do we want to silently allow changing config when specifying more than one type, but only one has pollers? kinda seems like a "warn first" scenario to me.
unless I'm misreading this?
There was a problem hiding this comment.
What you're describing is correct.
We pretty frequently partition a tasklist for both types despite only a single type being in use. Looking at our DynamicConfig we've never once partitioned a TaskList and used the TaskListType as one of the filters. So I think it provides better continuity and doesn't really create any problems for the system.
The intention behind this check is to catch cases where we specify the incorrect name. We recently had this happen with a large TaskList that we intended to change to 10 partitions but we partitioned task_list_name instead of task-list-name which caused some really bad hotsharding during the next monthly burst.
There was a problem hiding this comment.
yea, definitely useful either way. and makes sense if it's kinda common 👍
| slices.SortFunc(table, func(a, b TaskListRow) int { | ||
| return strings.Compare(a.Name, b.Name) | ||
| }) |
There was a problem hiding this comment.
I think you'll want slices.SortStableFunc here, or sort by both name and type. Sort is unstable, so you might see activity/decision reorders under the same name.
e.g. I think this is possible currently:
decision tasklist-A 3
activity tasklist-A 3
activity tasklist-B 3
decision tasklist-B 3
but SortStable will ensure it's always decision/activity/decision/activity (for same name pairs).
I don't think the tests exercise this, so it's rather minor at the moment, but probably still worth doing.
There was a problem hiding this comment.
I actually changed the table format entirely, it's now this:
| Name | Activity Pollers | Decision Pollers |
|---|---|---|
| tasklist-A | 3 | 3 |
| tasklist-B | 3 | 3 |
So the name is unique and we've aggregated the counts across both types. My goal was to align it more with the Cadence UI, which lists the pollers of a TaskList and has a checkbox for whether each has a Decision/Activity poller.
There was a problem hiding this comment.
ah, I see, it's just iteratively filling the same row in both loops.
yea that's fine then 👍
| output := td.consoleOutput() | ||
| var result map[types.TaskListType]*types.DescribeTaskListResponse | ||
| err := json.Unmarshal([]byte(output), &result) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, expected, result) |
| }, | ||
| }, nil).Times(1) | ||
| }, | ||
| expectedError: "Decision", |
There was a problem hiding this comment.
could probably use a more descriptive expected-error, or some kind of comment. this is rather vague about what it is checking, since "Decision" could be in lots of error messages.
While Activity/Decision TaskLists with a given name are independent, they typically represent a common purpose and we have never operated on just a single type. Handling them entirely separately within the admin commands doubles the amount of commands needed to run to operate on the TaskList and makes it harder to understand whether a given TaskList name is used for decision and/or activity tasks. Particularly as we move TaskList partition data from dynamic config to the DB it's important that we provide good operator ergonomics to avoid errors. Update the Admin TaskList commands with the following: - The absence of the `tasklisttype` flag now indicates both Activity and Decision. It previously was inconsistent between defaulting to Decision or being required. - The describe command will output one row per type specified rather than a single row. - The describe command will output that a TaskList has 1 read/write partition rather than no output if there is no config. - The describe command will no longer output the list of pollers and instead print out the number of pollers. This functionality is still available via the non-admin version of the command, which does nothing other than printing the pollers. - The describe command now supports json as an output format. - The list command will consolidate Activity and Decision TaskLists with a given name to a single row, including separate columns for the number of decision and activity pollers. - The list command now sorts its output by TaskList name. - The list command now supports json as an output format. - The update-partition command now operates on multiple TaskListTypes at once rather than Decision or Activity. It performs a safety check on both before performing any updates.
While Activity/Decision TaskLists with a given name are independent, they typically represent a common purpose and we have never operated on just a single type. Handling them entirely separately within the admin commands doubles the amount of commands needed to run to operate on the TaskList and makes it harder to understand whether a given TaskList name is used for decision and/or activity tasks. Particularly as we move TaskList partition data from dynamic config to the DB it's important that we provide good operator ergonomics to avoid errors.
What changed?
Update the Admin TaskList commands with the following:
tasklisttypeflag now indicates both Activity and Decision. It previously was inconsistent between defaulting to Decision or being required.Why?
How did you test it?
Potential risks
Release notes
Documentation Changes