scheduler WIP: add GetAllJobs api.

scheduler: add GetAllJobs api.

BUG=712427
R=vadimsh@chromium.org

Review-Url: https://codereview.chromium.org/2945843002
Committed: https://github.com/luci/luci-go/commit/e472357afe0b83c90c44004977799450e16966b9

[tandrii(chromium), 2017-06-19 13:51:40]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 13:51:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 13:58:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 13:58:02]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2017-06-19 17:44:19]

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
File scheduler/api/scheduler/v1/scheduler.proto (right):

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
scheduler/api/scheduler/v1/scheduler.proto:27: message JobWithState {
just "Job"

if we need it without state, we can set state to nil (it is optional field)

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
scheduler/api/scheduler/v1/scheduler.proto:36: string kind = 1;
so... there are actually two flavors of state: one is used internally in state
machine, another is shown in UI. UI states are functions of internal state, they
are more user-friendly. See
https://github.com/luci/luci-go/blob/master/scheduler/appengine/ui/presentati...

I think we might need return UI-level state in API too, to avoid confusing
people. Internal state can be kept as internal implementation detail.

https://codereview.chromium.org/2945843002/diff/1/scheduler/appengine/apiserv...
File scheduler/appengine/apiservers/scheduler.go (right):

https://codereview.chromium.org/2945843002/diff/1/scheduler/appengine/apiserv...
scheduler/appengine/apiservers/scheduler.go:13: context
"golang.org/x/net/context"
move this import to existing imports section, before luci-go imports

also no need to alias as "context", it is already "context"

https://codereview.chromium.org/2945843002/diff/1/scheduler/appengine/apiserv...
scheduler/appengine/apiservers/scheduler.go:34: return nil, err
gRPC methods should always return grpc errors, e.g. grpc.Errorf(codes.Internal,
"datastore error - %s", err)

[tandrii(chromium), 2017-06-20 11:48:04]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[tandrii(chromium), 2017-06-20 11:48:06]

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
File scheduler/api/scheduler/v1/scheduler.proto (right):

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
scheduler/api/scheduler/v1/scheduler.proto:27: message JobWithState {
On 2017/06/19 17:44:19, Vadim Sh. wrote:
> just "Job"
> 
> if we need it without state, we can set state to nil (it is optional field)

Done.

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
scheduler/api/scheduler/v1/scheduler.proto:36: string kind = 1;
On 2017/06/19 17:44:19, Vadim Sh. wrote:
> so... there are actually two flavors of state: one is used internally in state
> machine, another is shown in UI. UI states are functions of internal state,
they
> are more user-friendly. See
>
https://github.com/luci/luci-go/blob/master/scheduler/appengine/ui/presentati...
> 
> I think we might need return UI-level state in API too, to avoid confusing
> people. Internal state can be kept as internal implementation detail.

I agree that UI one is more useful; however, if internal implementation detail,
then why expose it? So I don't expose it any more, and instead just offer usable
`string state` in it. WDYT?

https://codereview.chromium.org/2945843002/diff/1/scheduler/appengine/apiserv...
File scheduler/appengine/apiservers/scheduler.go (right):

https://codereview.chromium.org/2945843002/diff/1/scheduler/appengine/apiserv...
scheduler/appengine/apiservers/scheduler.go:13: context
"golang.org/x/net/context"
On 2017/06/19 17:44:19, Vadim Sh. wrote:
> move this import to existing imports section, before luci-go imports
> 
> also no need to alias as "context", it is already "context"

Done.

TBH, i learned to ignore import sections in GO thanks to goimports, but I guess
i had some copy-pasta-caused issues.

https://codereview.chromium.org/2945843002/diff/1/scheduler/appengine/apiserv...
scheduler/appengine/apiservers/scheduler.go:34: return nil, err
On 2017/06/19 17:44:19, Vadim Sh. wrote:
> gRPC methods should always return grpc errors, e.g.
grpc.Errorf(codes.Internal,
> "datastore error - %s", err)

Done.

[commit-bot: I haz the power, 2017-06-20 11:48:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 11:54:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 11:54:18]

Dry run: This issue passed the CQ dry run.

[tandrii(chromium), 2017-06-20 13:41:02]

https://codereview.chromium.org/2945843002/diff/20001/scheduler/appengine/api...
File scheduler/appengine/apiservers/scheduler.go (right):

https://codereview.chromium.org/2945843002/diff/20001/scheduler/appengine/api...
scheduler/appengine/apiservers/scheduler.go:44: UiStatus:
string(ej.State.State),
hold on, I didn't actually make this use UI-friendly representation.

[tandrii(chromium), 2017-06-20 16:18:56]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 16:19:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2017-06-20 16:21:45]

ptal

[commit-bot: I haz the power, 2017-06-20 16:27:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 16:27:02]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2017-06-20 17:48:54]

looks great, some nits

you can try deploying this to luci-scheduler-dev:

gae.py upload -A luci-scheduler-dev

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
File scheduler/api/scheduler/v1/scheduler.proto (right):

https://codereview.chromium.org/2945843002/diff/1/scheduler/api/scheduler/v1/...
scheduler/api/scheduler/v1/scheduler.proto:36: string kind = 1;
On 2017/06/20 11:48:05, tandrii(chromium) wrote:
> On 2017/06/19 17:44:19, Vadim Sh. wrote:
> > so... there are actually two flavors of state: one is used internally in
state
> > machine, another is shown in UI. UI states are functions of internal state,
> they
> > are more user-friendly. See
> >
>
https://github.com/luci/luci-go/blob/master/scheduler/appengine/ui/presentati...
> > 
> > I think we might need return UI-level state in API too, to avoid confusing
> > people. Internal state can be kept as internal implementation detail.
> 
> I agree that UI one is more useful; however, if internal implementation
detail,
> then why expose it? So I don't expose it any more, and instead just offer
usable
> `string state` in it. WDYT?

Yes, I meant, the API should not expose internal job state.

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/api...
File scheduler/appengine/apiservers/scheduler.go (right):

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/api...
scheduler/appengine/apiservers/scheduler.go:56: func (s SchedulerServer)
getTaskTraits(ctx context.Context, j *engine.Job) task.Traits {
I think this should be moved somewhere else, so it can be reused from both
places. Maybe 'presentation' too?..

GetTaskTraits(cat *catalog.Catalog, j *engine.Job) (task.Traits, error)

(and return an error in case of failed deserialization or what not)

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
File scheduler/appengine/presentation/state.go (right):

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
scheduler/appengine/presentation/state.go:64: if r, ok :=
map[engine.StateKind]PublicStateKind{
please make this map package-level global var

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
File scheduler/appengine/presentation/state_test.go (right):

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
scheduler/appengine/presentation/state_test.go:39:
So(GetPublicStateKind(&engine.Job{
nit: \n :)

[tandrii(chromium), 2017-06-20 19:43:37]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 19:43:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2017-06-20 19:46:37]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 19:46:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2017-06-20 19:48:36]

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/api...
File scheduler/appengine/apiservers/scheduler.go (right):

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/api...
scheduler/appengine/apiservers/scheduler.go:56: func (s SchedulerServer)
getTaskTraits(ctx context.Context, j *engine.Job) task.Traits {
On 2017/06/20 17:48:54, Vadim Sh. wrote:
> I think this should be moved somewhere else, so it can be reused from both
> places. Maybe 'presentation' too?..
> 
> GetTaskTraits(cat *catalog.Catalog, j *engine.Job) (task.Traits, error)
> 
> (and return an error in case of failed deserialization or what not)

Done.

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
File scheduler/appengine/presentation/state.go (right):

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
scheduler/appengine/presentation/state.go:64: if r, ok :=
map[engine.StateKind]PublicStateKind{
On 2017/06/20 17:48:54, Vadim Sh. wrote:
> please make this map package-level global var

Done.

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
File scheduler/appengine/presentation/state_test.go (right):

https://codereview.chromium.org/2945843002/diff/60001/scheduler/appengine/pre...
scheduler/appengine/presentation/state_test.go:39:
So(GetPublicStateKind(&engine.Job{
On 2017/06/20 17:48:54, Vadim Sh. wrote:
> nit: \n :)

Done.

[tandrii(chromium), 2017-06-20 19:49:05]

On 2017/06/20 17:48:54, Vadim Sh. wrote:
> looks great, some nits
> 
> you can try deploying this to luci-scheduler-dev:
> 
> gae.py upload -A luci-scheduler-dev

https://1790-cd725e3-tainted-tandrii-dot-luci-scheduler-dev.appspot.com/ :)

[tandrii(chromium), 2017-06-20 19:52:07]

On 2017/06/20 19:49:05, tandrii(chromium) wrote:
> On 2017/06/20 17:48:54, Vadim Sh. wrote:
> > looks great, some nits
> > 
> > you can try deploying this to luci-scheduler-dev:
> > 
> > gae.py upload -A luci-scheduler-dev
> 
> https://1790-cd725e3-tainted-tandrii-dot-luci-scheduler-dev.appspot.com/ :)

Yay, explorer playing confirms my meager attempt is working:
https://1790-cd725e3-tainted-tandrii-dot-luci-scheduler-dev.appspot.com/rpcex...

[commit-bot: I haz the power, 2017-06-20 19:53:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 19:53:10]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2017-06-20 20:16:26]

lgtm

but update commit message to be something normal before committing

[tandrii(chromium), 2017-06-20 20:17:11]

Description was changed from

==========
scheduler WIP: add GetAllJobs api.

CL author is a noob, so comments are required... otherwise this noob
will implement & test the rest of API the same way.

BUG=712427
R=vadimsh@chromium.org
==========

to

==========
scheduler: add GetAllJobs api.

BUG=712427
R=vadimsh@chromium.org
==========

[tandrii(chromium), 2017-06-20 20:30:33]

On 2017/06/20 20:16:26, Vadim Sh. wrote:
> lgtm
> 
> but update commit message to be something normal before committing

Done.

[tandrii(chromium), 2017-06-20 20:30:38]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2017-06-20 20:30:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 20:36:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497990638406720,
"parent_rev": "8633317accf225c46c29f8cd3b12bfaff06f3f04", "commit_rev":
"e472357afe0b83c90c44004977799450e16966b9"}

[commit-bot: I haz the power, 2017-06-20 20:36:16]

Description was changed from

==========
scheduler: add GetAllJobs api.

BUG=712427
R=vadimsh@chromium.org
==========

to

==========
scheduler: add GetAllJobs api.

BUG=712427
R=vadimsh@chromium.org

Review-Url: https://codereview.chromium.org/2945843002
Committed:
https://github.com/luci/luci-go/commit/e472357afe0b83c90c44004977799450e16966b9
==========

[commit-bot: I haz the power, 2017-06-20 20:36:17]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/e472357afe0b83c90c44004977799450e16966b9