Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2946343004:
scheduler: add rpcs for actions on job and invocation. (Closed)
|
Created:
3 years, 6 months ago by tandrii(chromium) Modified:
3 years, 5 months ago Reviewers:
Vadim Sh. CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
Descriptionscheduler: add rpcs for actions on job and invocation.
Also:
* refactor API messages to use JobRef and InvocationRef consistently.
* introduce named errors to engine
BUG=712427
Review-Url: https://codereview.chromium.org/2946343004
Committed: https://github.com/luci/luci-go/commit/8315cf5fcf6a6048afd984cec0dc32a4027ff4da
Patch Set 1 #
Total comments: 12
Patch Set 2 : with tests, without error codes #Patch Set 3 : with errors #
Total comments: 6
Patch Set 4 : review #
Messages
Total messages: 36 (22 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tandrii@chromium.org changed reviewers: + vadimsh@chromium.org
PTAL the WIP % (no tests) High level questions: 1. TriggerInvocation returns nothing, because I don't imagine it being useful without the need to poll for actually running invocation anyway. SGT<you>? 2. What's the best way to "catch" JobNotFound error and alike? Introduce special Error constants to the engine? 3. Any other API you think is missing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
> 2. What's the best way to "catch" JobNotFound error and alike? Introduce special Error constants to the engine? Yeah, add ErrNoSuchJob (or something) and return it instead of adhoc fmt.Errorf(...) where appropriate. https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... File scheduler/api/scheduler/v1/scheduler.proto (right): https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:16: rpc GetJobs(JobsRequest) returns (JobsReply); does this return "is paused" flag (is it in "ui_state" perhaps)? https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:24: // This call likely returns before actual invocation is created. Therefore, this is shitty API from user POV :) It has to return some kind of an identifier (invocation nonce?) that can be polled on later, if desired. Maybe we can rename invocationNonce into "invocationOp" in the API layer (or everywhere.. "nonce" was a bad name) and have TriggerInvocation(...) -> returns invocationOp (int64) GetInvocations({..., invocationOp: ...}) -> returns all invocation that were initiated by this op (may be many due to retries). ---- Alternatively, just remove TriggerInvocation API for now. https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:117: message JobActionRequest { consider renaming this into JobRef message and using it consistently in API everywhere where a job is referenced https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:122: message InvocationActionRequest{ same here: InvocationRef
https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... File scheduler/api/scheduler/v1/scheduler.proto (right): https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:16: rpc GetJobs(JobsRequest) returns (JobsReply); On 2017/06/26 17:36:32, Vadim Sh. wrote: > does this return "is paused" flag (is it in "ui_state" perhaps)? not directly, but ui_state - should be. Worth exposing directly as boolean, similary to `final`? https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:24: // This call likely returns before actual invocation is created. Therefore, On 2017/06/26 17:36:32, Vadim Sh. wrote: > this is shitty API from user POV :) It has to return some kind of an identifier > (invocation nonce?) that can be polled on later, if desired. > > Maybe we can rename invocationNonce into "invocationOp" in the API layer (or > everywhere.. "nonce" was a bad name) and have > > TriggerInvocation(...) -> returns invocationOp (int64) > > GetInvocations({..., invocationOp: ...}) -> returns all invocation that were > initiated by this op (may be many due to retries). > > ---- > > Alternatively, just remove TriggerInvocation API for now. I also thought that it is crappy API at first, but I changed my mind. I think there is no reason to know exactly **which** invocation was triggered, as it won't be any different than say already scheduled invocation OR if another user called TriggerInvocation first. The reason I'd call this API is to ensure that at least one invocation has been scheduled and will soon be executed. Am I wrong | missing smth? that said, I don't feel strongly about having this API at all. I also don't mind exposing invocationOps, but how useful is that to api user, unless the goal is debugging some job?
https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... File scheduler/api/scheduler/v1/scheduler.proto (right): https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:16: rpc GetJobs(JobsRequest) returns (JobsReply); On 2017/06/26 18:33:57, tandrii(chromium) wrote: > On 2017/06/26 17:36:32, Vadim Sh. wrote: > > does this return "is paused" flag (is it in "ui_state" perhaps)? > > not directly, but ui_state - should be. Worth exposing directly as boolean, > similary to `final`? Dunno. Let's not for now. It is always easier to add stuff than remove it. https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:24: // This call likely returns before actual invocation is created. Therefore, On 2017/06/26 18:33:57, tandrii(chromium) wrote: > On 2017/06/26 17:36:32, Vadim Sh. wrote: > > this is shitty API from user POV :) It has to return some kind of an > identifier > > (invocation nonce?) that can be polled on later, if desired. > > > > Maybe we can rename invocationNonce into "invocationOp" in the API layer (or > > everywhere.. "nonce" was a bad name) and have > > > > TriggerInvocation(...) -> returns invocationOp (int64) > > > > GetInvocations({..., invocationOp: ...}) -> returns all invocation that were > > initiated by this op (may be many due to retries). > > > > ---- > > > > Alternatively, just remove TriggerInvocation API for now. > > I also thought that it is crappy API at first, but I changed my mind. I think > there is no reason to know exactly **which** invocation was triggered, as it > won't be any different than say already scheduled invocation OR if another user > called TriggerInvocation first. The reason I'd call this API is to ensure that > at least one invocation has been scheduled and will soon be executed. Am I wrong > | missing smth? > > that said, I don't feel strongly about having this API at all. I also don't mind > exposing invocationOps, but how useful is that to api user, unless the goal is > debugging some job? Let's remove TriggerInvocation unless someone actually needs it. Regardless of API, it has a potential for being abused.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WIP. scheduler: add rpcs for actions on job and invocation. BUG=712427 ========== to ========== scheduler: add rpcs for actions on job and invocation. Also: * refactor API messages to use JobRef and InvocationRef consistently. * introduce named errors to engine BUG=712427 ==========
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... File scheduler/api/scheduler/v1/scheduler.proto (right): https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:16: rpc GetJobs(JobsRequest) returns (JobsReply); On 2017/06/27 01:43:03, Vadim Sh. wrote: > On 2017/06/26 18:33:57, tandrii(chromium) wrote: > > On 2017/06/26 17:36:32, Vadim Sh. wrote: > > > does this return "is paused" flag (is it in "ui_state" perhaps)? > > > > not directly, but ui_state - should be. Worth exposing directly as boolean, > > similary to `final`? > > Dunno. Let's not for now. It is always easier to add stuff than remove it. Acknowledged. https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:24: // This call likely returns before actual invocation is created. Therefore, On 2017/06/27 01:43:03, Vadim Sh. wrote: > On 2017/06/26 18:33:57, tandrii(chromium) wrote: > > On 2017/06/26 17:36:32, Vadim Sh. wrote: > > > this is shitty API from user POV :) It has to return some kind of an > > identifier > > > (invocation nonce?) that can be polled on later, if desired. > > > > > > Maybe we can rename invocationNonce into "invocationOp" in the API layer (or > > > everywhere.. "nonce" was a bad name) and have > > > > > > TriggerInvocation(...) -> returns invocationOp (int64) > > > > > > GetInvocations({..., invocationOp: ...}) -> returns all invocation that were > > > initiated by this op (may be many due to retries). > > > > > > ---- > > > > > > Alternatively, just remove TriggerInvocation API for now. > > > > I also thought that it is crappy API at first, but I changed my mind. I think > > there is no reason to know exactly **which** invocation was triggered, as it > > won't be any different than say already scheduled invocation OR if another > user > > called TriggerInvocation first. The reason I'd call this API is to ensure that > > at least one invocation has been scheduled and will soon be executed. Am I > wrong > > | missing smth? > > > > that said, I don't feel strongly about having this API at all. I also don't > mind > > exposing invocationOps, but how useful is that to api user, unless the goal is > > debugging some job? > > Let's remove TriggerInvocation unless someone actually needs it. Regardless of > API, it has a potential for being abused. Done. https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:117: message JobActionRequest { On 2017/06/26 17:36:32, Vadim Sh. wrote: > consider renaming this into JobRef message and using it consistently in API > everywhere where a job is referenced Done. I might have gone too far, if so, lmk -> i'll go back somewhat :) https://codereview.chromium.org/2946343004/diff/1/scheduler/api/scheduler/v1/... scheduler/api/scheduler/v1/scheduler.proto:122: message InvocationActionRequest{ On 2017/06/26 17:36:32, Vadim Sh. wrote: > same here: InvocationRef Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit don't forget to update CL description before committing https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... File scheduler/appengine/apiservers/scheduler.go (right): https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... scheduler/appengine/apiservers/scheduler.go:8: scheduler "github.com/luci/luci-go/scheduler/api/scheduler/v1" nit: remove this alias, it is already "scheduler". https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... scheduler/appengine/apiservers/scheduler.go:138: if !presentation.IsJobOwner(ctx, jobRef.GetProject(), jobRef.GetJob()) { strictly speaking, this function should be in 'acl' module, not 'presentation'. We can move it later. https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... scheduler/appengine/apiservers/scheduler.go:143: return nil, nil return &empty.Empty{}, nil (i think...)
what's wrong with description? https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... File scheduler/appengine/apiservers/scheduler.go (right): https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... scheduler/appengine/apiservers/scheduler.go:8: scheduler "github.com/luci/luci-go/scheduler/api/scheduler/v1" On 2017/07/05 18:38:48, Vadim Sh. wrote: > nit: remove this alias, it is already "scheduler". Done. I keep adding it if I open my vim configured with goimports because otherwise it's removed. And the reason, i guess is i forget to get into go env :( https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... scheduler/appengine/apiservers/scheduler.go:138: if !presentation.IsJobOwner(ctx, jobRef.GetProject(), jobRef.GetJob()) { On 2017/07/05 18:38:48, Vadim Sh. wrote: > strictly speaking, this function should be in 'acl' module, not 'presentation'. > We can move it later. Agree, added TODO in acl.go. Will execute on it once I get to ACLs. https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... scheduler/appengine/apiservers/scheduler.go:143: return nil, nil On 2017/07/05 18:38:47, Vadim Sh. wrote: > return &empty.Empty{}, nil > > (i think...) that's good point. I've not tested it with rpcexplorer bcz it doesn't work locally.
On 2017/07/05 19:21:51, tandrii(chromium) SLOW wrote: > what's wrong with description? Ah, it's just title! I changed description, but Rietveld keeps title intact.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/05 19:21:51, tandrii(chromium) SLOW wrote: > what's wrong with description? > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > File scheduler/appengine/apiservers/scheduler.go (right): > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > scheduler/appengine/apiservers/scheduler.go:8: scheduler > "github.com/luci/luci-go/scheduler/api/scheduler/v1" > On 2017/07/05 18:38:48, Vadim Sh. wrote: > > nit: remove this alias, it is already "scheduler". > > Done. > I keep adding it if I open my vim configured with goimports because otherwise > it's removed. And the reason, i guess is i forget to get into go env :( > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > scheduler/appengine/apiservers/scheduler.go:138: if > !presentation.IsJobOwner(ctx, jobRef.GetProject(), jobRef.GetJob()) { > On 2017/07/05 18:38:48, Vadim Sh. wrote: > > strictly speaking, this function should be in 'acl' module, not > 'presentation'. > > We can move it later. > > Agree, added TODO in acl.go. Will execute on it once I get to ACLs. > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > scheduler/appengine/apiservers/scheduler.go:143: return nil, nil > On 2017/07/05 18:38:47, Vadim Sh. wrote: > > return &empty.Empty{}, nil > > > > (i think...) > > that's good point. I've not tested it with rpcexplorer bcz it doesn't work > locally. What doesn't work locally exactly? RPC explorer? Or Scheduler? (Both should work locally...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2946343004/#ps60001 (title: "review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/05 19:23:47, Vadim Sh. wrote: > On 2017/07/05 19:21:51, tandrii(chromium) SLOW wrote: > > what's wrong with description? > > > > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > > File scheduler/appengine/apiservers/scheduler.go (right): > > > > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > > scheduler/appengine/apiservers/scheduler.go:8: scheduler > > "github.com/luci/luci-go/scheduler/api/scheduler/v1" > > On 2017/07/05 18:38:48, Vadim Sh. wrote: > > > nit: remove this alias, it is already "scheduler". > > > > Done. > > I keep adding it if I open my vim configured with goimports because otherwise > > it's removed. And the reason, i guess is i forget to get into go env :( > > > > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > > scheduler/appengine/apiservers/scheduler.go:138: if > > !presentation.IsJobOwner(ctx, jobRef.GetProject(), jobRef.GetJob()) { > > On 2017/07/05 18:38:48, Vadim Sh. wrote: > > > strictly speaking, this function should be in 'acl' module, not > > 'presentation'. > > > We can move it later. > > > > Agree, added TODO in acl.go. Will execute on it once I get to ACLs. > > > > > https://codereview.chromium.org/2946343004/diff/40001/scheduler/appengine/api... > > scheduler/appengine/apiservers/scheduler.go:143: return nil, nil > > On 2017/07/05 18:38:47, Vadim Sh. wrote: > > > return &empty.Empty{}, nil > > > > > > (i think...) > > > > that's good point. I've not tested it with rpcexplorer bcz it doesn't work > > locally. > > What doesn't work locally exactly? RPC explorer? Or Scheduler? (Both should work > locally...) RPC explorer, but turns out I tried loading it directly on static serving port, which doesn't work. If instead I use dispatcher's port, then I all works fine. I dunno why I didn't think of doing this before...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499284244038460,
"parent_rev": "778ac0b55880b89da16bcddae98765f45d08d3c7", "commit_rev":
"8315cf5fcf6a6048afd984cec0dc32a4027ff4da"}
Message was sent while issue was closed.
Description was changed from ========== scheduler: add rpcs for actions on job and invocation. Also: * refactor API messages to use JobRef and InvocationRef consistently. * introduce named errors to engine BUG=712427 ========== to ========== scheduler: add rpcs for actions on job and invocation. Also: * refactor API messages to use JobRef and InvocationRef consistently. * introduce named errors to engine BUG=712427 Review-Url: https://codereview.chromium.org/2946343004 Committed: https://github.com/luci/luci-go/commit/8315cf5fcf6a6048afd984cec0dc32a4027ff4da ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://github.com/luci/luci-go/commit/8315cf5fcf6a6048afd984cec0dc32a4027ff4da |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
