[vtadmin] GetWorkflow(s) endpoints#7662
Conversation
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
(I swear I know how the alphabet works .... maybe ......) Signed-off-by: Andrew Mason <amason@slack-corp.com>
go/vt/vtadmin/cluster/cluster.go
Outdated
| return c.findWorkflows(ctx, keyspaces, opts) | ||
| } | ||
|
|
||
| func (c *Cluster) findWorkflows(ctx context.Context, keyspaces []string, opts FindWorkflowsOptions) ([]*vtadminpb.Workflow, error) { |
There was a problem hiding this comment.
Note to reviewers: this function is the bulk of the logic; all (non-error) codepaths end up calling this function.
| }, nil | ||
| } | ||
|
|
||
| func (api *API) getClustersForRequest(ids []string) ([]*cluster.Cluster, []string) { |
There was a problem hiding this comment.
just moving this to the bottom of the file; it felt a bit out of place being stuck between two public functions without really being a helper function for either of them in particular
ajm188
force-pushed
the
am_vtadmin_getworkflows
branch
2 times, most recently
from
b776838 to
f38ace3
Compare
Signed-off-by: Andrew Mason <amason@slack-corp.com>
… the param name Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188
force-pushed
the
am_vtadmin_getworkflows
branch
from
f38ace3 to
7b9e498
Compare
doeg
left a comment
doeg
left a comment
There was a problem hiding this comment.
Thank you so much!!! This is perfect.
| Name: "workflow1", | ||
| }, | ||
| expected: nil, | ||
| shouldErr: true, |
There was a problem hiding this comment.
Just curious -- in tests I have been defining an expectedError property on tests, and then asserting on errors.Is. This seems like it offers more granularity than shouldErr, but maybe it's not possible to do in all cases?
There was a problem hiding this comment.
My thinking is generally that if I care about the actual class of error (which is probably only true if I add a doc comment along the lines of "this function returns an <error type> when <condition>") then I will do the expectedErr + errors.Is. If I just care that it returns some error, then I'll do this.
| // Callers should use this function when they want more fine-grained filtering, | ||
| // and GetWorkflows when they just want to filter on keyspace name. | ||
| // | ||
| // Note that if only a subset of keyspaces error on their vtctld GetWorkflows |
There was a problem hiding this comment.
I can see what you mean about this being a little Complicated but FWIW it makes sense to me + works perfectly for the front-end. 😎
| // Keyspaces is a list of keyspaces to restrict the workflow search to. Note | ||
| // that the keyspaces list applies across all cluster IDs in the request. | ||
| // | ||
| // If, for example, you have two clusters, each with a keyspace called "foo" |
There was a problem hiding this comment.
This makes sense + thank you for documenting it so clearly!
| // Its route is /workflows, with query params: | ||
| // - cluster: repeated, cluster IDs | ||
| // - active_only | ||
| // - keyspace: repeated | ||
| // - ignore_keyspace: repeated |
There was a problem hiding this comment.
Sometimes... I have gross thoughts about somehow... adding protos for our HTTP request shapes, since I end up redefining the types on the front-end anyway. Probably our time would be better spent on gRPC-on-the-front-end... 😭 ...unless...............
There was a problem hiding this comment.
(my vote is also for grpc-on-the-front-end, some day ................)
|
|
||
| // AssertClusterWorkflowsEqual is a test helper for asserting that two | ||
| // ClusterWorkflows objects are equal. | ||
| func AssertClusterWorkflowsEqual(t *testing.T, expected *vtadminpb.ClusterWorkflows, actual *vtadminpb.ClusterWorkflows, msgAndArgs ...interface{}) { |
4 participants
Description
This PR adds two endpoints to the vtadmin api -
GetWorkflow, which returns a single Workflow for a given(cluster, keyspace, name); andGetWorkflows, which returns multiple workflows across multiple clusters according to a bunch of filtering criteria.For
GetWorkflows, you can pass the following options:Keyspaces: list of keyspaces to restrict the workflow search to. this applies across all clusters, so for finer-grained controls (e.g. "keyspace 'foo' in cluster1, but ignore keyspace 'foo' in cluster2", you'll need to make two separate requests)IgnoreKeyspaces: inverse of keyspaces, but is completely ignored if theKeyspacesoption is non-empty. Same caveat about applying across all clusters as theKeyspacesmentioned above.Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: