Initial distributor implementation

Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors, distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of those, ActivateExecution and FinishAttempt are very simple, and EnsureGraphData and WalkGraph have standardized on GraphData as their output format.
  * The persistent-state API has been implemented as part of the distributor interface.
  * The state machine flow has been simplified (elimination of 'adding deps' state, attempts now are in the executing state for the full duration of an outstanding execution, all failure states between attempts and executions are unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps tests were getting really annoying to update and keep correct.

BUG=550684
Committed: https://github.com/luci/luci-go/commit/00e902769955ef55f947ae046b1b530f5498241e

[iannucci, 2016-01-19 21:57:34]

iannucci@chromium.org changed reviewers:
+ dnj@chromium.org, estaab@chromium.org, martiniss@chromium.org,
nodir@chromium.org

[iannucci, 2016-01-19 21:57:35]

This is a complete WIP... doesn't work yet. BUT! If you could take a look at
appengine/cmd/dm/distributor/distributor.go, it has the description of the API
and responsibilities for a 'distributor'.

Please ignore the parser stuff... it's stupid and is going to go away (to be
replaced with a proto file).

[martiniss, 2016-01-19 23:02:40]

Just looked at the docs, should I look at more?

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
File appengine/cmd/dm/distributor/distributor.go (right):

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:9: //   * When an Execution for that
Quest NeedsExecution, DM reads configuration
reads *its* configuration.

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:15: //     record its Token.
Additionally, the distributor MUST subscribe to publish
Split into another bullet point? 

Also I don't really understand the phrasing "subscribe to publish on DM's pubsub
topic for updates", specifically "subscribe to publish". Does that just mean
subscribe? Or it looks like publish to that channel... Maybe I just don't know
pubsub that well.

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:19: //   * When DM gets a hit on
pubsub, it will load the Execution, load its cached
gets a message?

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:24: //   * Add a new subdir of
protos with the configuration proto for the new
Maybe add the directory itself you need, relative to here?

[nodir, 2016-01-20 21:07:32]

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
File appengine/cmd/dm/distributor/distributor.go (right):

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:40:
"github.com/luci/luci-go/appengine/cmd/dm/types"
I thought we keep the luci-go import section last

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:62: // Dep represents the pertinent
information of a single dependency.
persistent

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:106: // Rejected with the returned
error message as the 'Reason'.
What is Reason? Please reference it so a reader can find it

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:119: GetStatus(Token) (*TaskState,
error)
rename to "State"?

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:125: InfoURL(Token) string
could return *url.URL

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:140: HandleTaskQueueTask(c
context.Context, r *http.Request) error
HandlePushTask?

https://codereview.chromium.org/1537883002/diff/40001/appengine/cmd/dm/distri...
appengine/cmd/dm/distributor/distributor.go:143: // Factory is a function which
produces new distributor instance with the
nit: a new distributor

[martiniss, 2016-03-16 01:10:35]

is this still a thing?

[iannucci, 2016-06-08 02:54:24]

PTAL, will update description shortly.

Sorry this is huge :(. If you want me to do an over the shoulder review with
you, please let me know. I couldn't find a clean way to split it into smaller
reviews though that still make sense.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/pubsub.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/pubsub.go:24: func pubsubTopic(c context.Context)
string {
note that all of this is roughly untested. This will be fixed at the beginning
of the actual swarming integration changes.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/test_registry.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/test_registry.go:26: func NewTestingRegistry(mocks
map[string]D, fFn FinishExecutionFn) Registry {
this evades luci-config/pubsub in tests.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/model/attempt.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/attempt.go:89: DepMap bf.BitField `gae:",noindex"
json:"-"`
this is part of the state machine simplification; there's no real need to
distinguish between 'adding deps' and 'blocked'. Tumble adds backdeps in the
background, but the state of THIS attempt is just 'waiting'. It can't proceed
from waiting until (all backdeps added && dependencies finished), which is
enforced through the mutations.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/attempt.go:141: case dm.Attempt_SCHEDULING:
these changes are due to the state machine simplification as well.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/model/execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/execution.go:59: TimeToStop  time.Duration
`gae:",noindex"` // STOPPING -> FINISHED
timeouts!

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/finish_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/finish_execution.go:23: type FinishExecution struct {
this is the major state machine change. Previously:
  * An execution was started at the time any dependency was taken on an attempt
which did not exist, or an attempt went from blocked->unblocked
  * An execution would finish 'sometime after' the FinishAttempt rpc or
EnsureGraphData rpc, but the completion of the execution wasn't really
well-tracked.

With the addition of the 'state' api, an Attempt is "EXECUTING" any time it has
an unfinished execution. This means that even after FinishAttempt happens, we
still wait for the execution to finish (e.g. for swarming to tell DM that the
job is done). As part of the distributor telling DM that the job is done, it
also updates the 'persistent state' for the attempt (for swarming, this is the
isolated_outdir hash for the task). If the task fails between the FinishAttempt
rpc and the distributor telling DM about it, we treat the execution as failed,
and it's eligible for a retry.

The reduction in complexity comes from the fact that an Attempt is now
completely blocked on its execution completing; it can't fire another execution
or otherwise change state until the current execution completes (or times out).

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/schedule_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/schedule_execution.go:33: func (s *ScheduleExecution)
RollForward(c context.Context) (muts []tumble.Mutation, err error) {
this is the other beefish change: now when a ScheduleExecution mutation occurs
it actually loads the distributor for the execution and makes the rpc to the
distributor, recording the distributor token to the execution.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/graph_data.proto (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:16: message AbnormalFinish {
Here are all the abnormal finish status types. The 'AbnormalFinish' is distinct
from 'Finished'. 'Finished' is always 100% OK. AbnormalFinish is everything
else.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:27: FAILED = 0;
I'm not super sure how to deal with this one: this is a swarming concept that
I've ported to DM. Basically if the distributor says that it's "failed", then
that shows up here. However, the way I plan to implement things in recipe
engine, the recipe itself should never exit non-0 (unless shit->fan). So in
normal recipe-based operation, we shouldn't actually see this.

The recipe engine would instead actually encapsulate the type of failure in its
normal json result blob (precise format TBD). This way depend_on could make
intelligent decisions about retries and such. And, of course, if the user wants
to have more that one failure flag, or status code (enum), they can include that
in the job result as well.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:68: CANCELLED = 4;
later when DM supports cancellation directly, this status will be used.

[iannucci, 2016-06-08 03:01:16]

Description was changed from

==========
Distributor WIP

BUG=
==========

to

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.
  * an 'Include.All' option was added to the WalkGraph options.

BUG=550684
==========

[iannucci, 2016-06-08 03:18:11]

Description was changed from

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.
  * an 'Include.All' option was added to the WalkGraph options.

BUG=550684
==========

to

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple, and EnsureGraphData
and WalkGraph have standardized on GraphData as their output format.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.
  * an 'Include.All' option was added to the WalkGraph options.

BUG=550684
==========

[dnj (Google), 2016-06-09 18:00:49]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2016-06-09 18:00:58]

Looks generally good and straightforward. I have a few questions/comments.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/deps/ensure_graph_data.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/deps/ensure_graph_data.go:300: fmt.Errorf("Quest %d:%q must
have a matching Attempts entry", i, q.ID),
Maybe use annotated error?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/deps/walk_graph.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/deps/walk_graph.go:130: func loadInfoURL(c context.Context, reg
distributor.Registry, dat *dm.Execution_Data_DistributorInfo) {
WDYT about actually returning an error here and logging/ignoring it at the call
site? That or rename this "maybeLoadInfoURL".

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/deps/walk_graph.go:183: if !exist.Any() {
nit: I would use "All" here. Obviously either works, though.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/distributor.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/distributor.go:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
nit: LUCI license (here and elsewhere)

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/distributor.go:57: PersistentState PersistentState
nit: "If PersistentState"

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/distributor.go:104: // pubsub notification
pipeline.
Maybe mention that delays over 10s have occasionally been observed in Pub/Sub?
I'd suggest a time here, maybe "12hr"?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/distributor.go:152: // json object. This should
validate that the content of that json object is
nit: JSON

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/fake/fake.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:143: func (t *ActivatedTask) DepOn(to
...*dm.Attempt_ID) (halt bool, err error) {
nit: don't use named return parameters here if you're not going to assign to
them.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:159: // Finish calls FinishAttempt
with the provided json body and optional
nit: JSON

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:169: if len(expire) > 0 {
Suggest: switch len(expire) { case 0: case 1: default: panic() }

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:222: if f.RunError != nil {
Suggest:
if err = f.RunError; err != nil { return }

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:230: f.Lock()
nit: do all task creation outside of Lock, save Lock for f.tasks manipulation.
nit: defer f.Unlock()

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:251: if tsk, ok := f.tasks[tok]; ok {
nit: defer f.Unlock()

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:257: err = fmt.Errorf("MISSING task
%s", tok)
nit: %q

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:265: f.Lock()
nit: defer f.Unlock()

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:401: JsonPayload:          
fmt.Sprintf(`{"name": "%s"}`, name),
This will have weird escaping issues if "name" has special characters in it. I
think you should do:

d, err := json.Marshal(struct{Name string `json:"name"`}{name})
panicIf(err)

...
JsonPayload: string(d)

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/fake/fake.go:440: r := request.(*dm.WalkGraphReq)
I guess you should check "expect" length here.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/handlers.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/handlers.go:22: TaskqueueHandler(WithRegistry(c,
reg), rw, r, p)
nit: Looks like we're using "TaskQueue" instead of "Taskqueue"

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/notify_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/notify_execution.go:33: logging.Fields{"error":
err, "cfg": f.CfgName}.Errorf(c, "Failed to make distributor")
nit: logging.ErrorKey

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/pubsub.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/pubsub.go:25: return
fmt.Sprintf("projects/%s/topics/%s", getTrimmedAppID(c), notifyTopicSuffix)
I would recommend using "common/gcloud/pubsub" here.

You can then call:
pubsub.NewTopic()

It's typed, you can validate, etc.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/pubsub.go:28: // PubsubReciever is the http handler
that processes incoming pubsub events
nit: HTTP

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/pubsub.go:34: func PubsubReciever(c
context.Context, rw http.ResponseWriter, r *http.Request, p httprouter.Params) {
nit: it might be cleaner to wrap a more specific method:

func handlePubSub(c context.Context, body []byte) (ack bool) {...}

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/pubsub.go:48: err :=
json.NewDecoder(r.Body).Decode(psm)
Why use JSON here and not protobuf?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/pubsub.go:49: if err != nil {
nit: if err := json.NewDecoder...

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/registry.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:22: var regKey = "holds a Registry"
nit: "holds a DM Distributor Registry"

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:27: return c
Perhaps this should panic? I think that this is clearly a bug if this happens.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:95: r.finishExecutionImpl = i
This seems like it would need locking. But it's not part of the interface...

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:122: func getTrimmedAppID(c
context.Context) string {
Maybe this should be built into gae/info?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:128: // loadConfig loads the named
distributor configurtaion from luci-config,
nit: "configuration"

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:132: cfgSvc := config.Get(c)
Note: You allow a nil config service in "init()", so this could be nil.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/registry.go:179: scheme + "://" +
inf.DefaultVersionHostname() + "/",
I think you should use a net.URL here, then call its String() method.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/task_description.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/task_description.go:56: ret := *t.payload
I think you should build "clone" into "Quest_Desc".

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/distributor/test_registry.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/distributor/test_registry.go:20: var _ Registry =
(*testRegistry)(nil)
nit: this isn't necessary, since NewTestingRegistry does this assertion.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/frontend/app.yaml (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/frontend/app.yaml:18: LUCI_DM_CONFIG_BASE_PATH:
/s/infra/infra/go/src/github.com/luci/luci-go/appengine/cmd/dm/integration_tests/cfgdir/current
hrm? Should probably not commit.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/model/attempt.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/attempt.go:30: a.Failed = 0
*a = AttemptRetryState{} !

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/attempt.go:53: AbnormalFinish dm.AbnormalFinish
Why not make these pointers?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/model/execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/execution.go:85: Attempt:
datastore.Get(c).MakeKey("Attempt", e.AttemptID().DMEncoded()),
AttemptKeyFromID?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/execution.go:130: // Revoke will null-out the Token and
Put this Execution to the datastore. This
nit: "nil-out" in above documentation, "null-out" here, I prefer "clear" :)

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/execution.go:186: code = codes.Unauthenticated
Does this make sense? Unauthenticated specifically means that the user failed to
supply authentication.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/model/quest.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/model/quest.go:23: func NewQuest(c context.Context, desc
*dm.Quest_Desc) *Quest {
It might not be a bad idea to omit "c" here and supply "now" externally. At
worst this causes the same amount of work, and at best it makes this function
purely operations on data and allows "Now" to be called fewer times if the
caller already gets it. Just a thought.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/activate_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/activate_execution.go:16: // ActivateExecution
executions an execution, moving it from the
nit: executions an execution!

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/add_deps.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/add_deps.go:72: mergeQuestMap :=
map[string]*MergeQuest{}
nit: avoid an allocation if not needed by surrounding with "len(a.Quests) > 0".
Can also allocate to len(a.Quests), no?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/add_deps.go:98: for _, mut := range mergeQuestMap {
Any chance this will ever exceed maximum datastore transaction size? It looks
like it's pretty unbounded ATM.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/finish_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/finish_execution.go:78: if a.State ==
dm.Attempt_EXECUTING && a.CurExecution == f.EID.Id && !e.State.Terminal() {
For indentation purposes, maybe invert this and return early?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/schedule_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/schedule_execution.go:66: distTok, e.TimeToStart,
e.TimeToRun, e.TimeToStop, err = dist.Run(
Since it's possible for the transaction to fail, it's also possible that this
request will be made to the distributor several times. I suppose the idea is
that if this is a problem, the distributor can deduplicate requests based on the
parameters here if it wants?

How much do we care whether the distributor token is unique? If multiple
executions share the same token, do we finish all of them when we get a
completion notification?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
File appengine/cmd/dm/mutate/timeout_execution.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/timeout_execution.go:81: }.Errorf(c, "Could not
MakeDistributor")
Does this mean that if we ever unregister a Distributor, all of its pending
executions will never timeout because this Mutation cannot advance? Maybe we
need explicit "ErrDistributorDoesntExistDude" handling for this case. Or,
alternatively, count this towards our maximum timeout attempts.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/timeout_execution.go:86: if err != nil || rslt == nil &&
t.TimeoutAttempt < maxTimeoutAttempts {
rslt will never be nil here. Do you mean realRslt?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/timeout_execution.go:89: "task_result":     rslt,
(same here)

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/timeout_execution.go:100: muts = append(muts, t)
I think you'll like new Tumble API for this one :)

https://chromiumcodereview.appspot.com/1537883002/diff/440001/appengine/cmd/d...
appengine/cmd/dm/mutate/timeout_execution.go:104: if err != nil {
Can this ever be hit? You handle "err != nil" above in an OR, and I don't see it
getting changed since then.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/graph_data.proto (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:27: FAILED = 0;
On 2016/06/08 02:54:24, iannucci wrote:
> I'm not super sure how to deal with this one: this is a swarming concept that
> I've ported to DM. Basically if the distributor says that it's "failed", then
> that shows up here. However, the way I plan to implement things in recipe
> engine, the recipe itself should never exit non-0 (unless shit->fan). So in
> normal recipe-based operation, we shouldn't actually see this.
> 
> The recipe engine would instead actually encapsulate the type of failure in
its
> normal json result blob (precise format TBD). This way depend_on could make
> intelligent decisions about retries and such. And, of course, if the user
wants
> to have more that one failure flag, or status code (enum), they can include
that
> in the job result as well.

I think things pre-recipe-engine can fail (e.g., bad CIPD/Isolate hash in
description, disk full, etc.). This sort of thing is probably retryable (error
on the side of caution).

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:68: CANCELLED = 4;
On 2016/06/08 02:54:24, iannucci wrote:
> later when DM supports cancellation directly, this status will be used.

nit:
In American English, the verb cancel is usually inflected canceled and
canceling—with one l. This is not a rule, however, and exceptions are easily
found. In varieties of English from outside the U.S., including Canadian,
British, and Australian English, cancelled and cancelling are the preferred
spellings.

We're in 'Murica, son.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:75: // Attempts: the last Execution
had a REJECTED Status.
Is this not retryable? If we're out of quota, we might not be later. Maybe we
need a specific "out of quota" failure reason vs. "your job is bad and you
should feel bad."

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:83: // Attempts: the last Execution
had a REJECTED Status.
MISSING Status?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:183: // The Attempt is waiting to be
Executed
nit: period at end.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:187: EXECUTING = 1;
IMO: s/the current/its current/

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:195: // The Attempt is in an abnormal
final state
nit: period at end.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:226: string persistent_state_result =
4;
Is "bytes" more appropriate? "string" implies UTF8, which (given
transformations) is obviously possible, but maybe not the most appropriate form.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/graph_data_purge_timestamps.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data_purge_timestamps.go:7: // TimestampPurger is
for testing: invoking this on a struct in this package
I assume you have a good reason for this, so just a comment:

In general, I'd prefer you not do this. Since we have a test clock, we can
control timestamps easily, and differences in timestamp expectations can point
out real problems.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/quest_desc.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/quest_desc.go:5: package dm
*yawn*

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/quest_desc_normalize.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/quest_desc_normalize.go:65: return
base64.URLEncoding.EncodeToString(h[:])[:QuestIDLength]
Note that if we ever change QuestIDLength, data could be hard to reach. TBH I'd
prefer we stick with full SHA256.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/service.proto (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/service.proto:18: // exposes DM's Dependency Server
API.
Why no caps?

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/walk_graph.proto (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/walk_graph.proto:132: bool all = 128;
(I don't like this. If you ever add new fields that you don't want to "all",
this could become weird. Plus, this runs the risk of lazy folk simply wasting
resources. I don't think it's unreasonable to ask people to just set the things
they want to "true".)

[martiniss, 2016-06-13 23:56:41]

https://codereview.chromium.org/1537883002/diff/460001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/distributor.go (right):

https://codereview.chromium.org/1537883002/diff/460001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:31: //   * In the DM frontend,
import your new package implementation and run its
Filename you need to do this in? Or is that too fragile.

https://codereview.chromium.org/1537883002/diff/460001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:58: // If this is !nil, the
Exectuion is counted as ABNORMAL_FINISHED, with this
typo

[iannucci, 2016-06-15 00:46:02]

PTAL

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/deps/...
File appengine/cmd/dm/deps/ensure_graph_data.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/deps/...
appengine/cmd/dm/deps/ensure_graph_data.go:300: fmt.Errorf("Quest %d:%q must
have a matching Attempts entry", i, q.ID),
On 2016/06/09 18:00:54, dnj (Google) wrote:
> Maybe use annotated error?

I will do so in a followup for sure!

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/deps/...
File appengine/cmd/dm/deps/walk_graph.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/deps/...
appengine/cmd/dm/deps/walk_graph.go:130: func loadInfoURL(c context.Context, reg
distributor.Registry, dat *dm.Execution_Data_DistributorInfo) {
On 2016/06/09 18:00:54, dnj (Google) wrote:
> WDYT about actually returning an error here and logging/ignoring it at the
call
> site? That or rename this "maybeLoadInfoURL".

will rename, since we only call this in one place :)

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/deps/...
appengine/cmd/dm/deps/walk_graph.go:183: if !exist.Any() {
On 2016/06/09 18:00:54, dnj (Google) wrote:
> nit: I would use "All" here. Obviously either works, though.

yeah changed w/ rebarse

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/distributor.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/06/09 18:00:54, dnj (Google) wrote:
> nit: LUCI license (here and elsewhere)

Done, thanks.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:57: PersistentState PersistentState
On 2016/06/09 18:00:54, dnj (Google) wrote:
> nit: "If PersistentState"

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:104: // pubsub notification
pipeline.
On 2016/06/09 18:00:54, dnj (Google) wrote:
> Maybe mention that delays over 10s have occasionally been observed in Pub/Sub?
> I'd suggest a time here, maybe "12hr"?

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:152: // json object. This should
validate that the content of that json object is
On 2016/06/09 18:00:54, dnj (Google) wrote:
> nit: JSON

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/fake/fake.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:143: func (t *ActivatedTask) DepOn(to
...*dm.Attempt_ID) (halt bool, err error) {
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: don't use named return parameters here if you're not going to assign to
> them.

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:159: // Finish calls FinishAttempt
with the provided json body and optional
On 2016/06/09 18:00:54, dnj (Google) wrote:
> nit: JSON

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:169: if len(expire) > 0 {
On 2016/06/09 18:00:55, dnj (Google) wrote:
> Suggest: switch len(expire) { case 0: case 1: default: panic() }

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:222: if f.RunError != nil {
On 2016/06/09 18:00:55, dnj (Google) wrote:
> Suggest:
> if err = f.RunError; err != nil { return }

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:230: f.Lock()
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: do all task creation outside of Lock, save Lock for f.tasks manipulation.
> nit: defer f.Unlock()

done. this testing code changed a bunch

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:251: if tsk, ok := f.tasks[tok]; ok {
On 2016/06/09 18:00:54, dnj (Google) wrote:
> nit: defer f.Unlock()

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:257: err = fmt.Errorf("MISSING task
%s", tok)
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: %q

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:265: f.Lock()
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: defer f.Unlock()

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:401: JsonPayload:          
fmt.Sprintf(`{"name": "%s"}`, name),
On 2016/06/09 18:00:55, dnj (Google) wrote:
> This will have weird escaping issues if "name" has special characters in it. I
> think you should do:
> 
> d, err := json.Marshal(struct{Name string `json:"name"`}{name})
> panicIf(err)
> 
> ...
> JsonPayload: string(d)

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/fake/fake.go:440: r := request.(*dm.WalkGraphReq)
On 2016/06/09 18:00:55, dnj (Google) wrote:
> I guess you should check "expect" length here.

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/handlers.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/handlers.go:22: TaskqueueHandler(WithRegistry(c,
reg), rw, r, p)
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: Looks like we're using "TaskQueue" instead of "Taskqueue"

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/notify_execution.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/notify_execution.go:33: logging.Fields{"error":
err, "cfg": f.CfgName}.Errorf(c, "Failed to make distributor")
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: logging.ErrorKey

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/pubsub.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/pubsub.go:25: return
fmt.Sprintf("projects/%s/topics/%s", getTrimmedAppID(c), notifyTopicSuffix)
On 2016/06/09 18:00:55, dnj (Google) wrote:
> I would recommend using "common/gcloud/pubsub" here.
> 
> You can then call:
> pubsub.NewTopic()
> 
> It's typed, you can validate, etc.

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/pubsub.go:28: // PubsubReciever is the http handler
that processes incoming pubsub events
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: HTTP

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/pubsub.go:34: func PubsubReciever(c
context.Context, rw http.ResponseWriter, r *http.Request, p httprouter.Params) {
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: it might be cleaner to wrap a more specific method:
> 
> func handlePubSub(c context.Context, body []byte) (ack bool) {...}

meh.... all this handler stuff is already function soup. I'm going to err on the
'less indirection' side here.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/pubsub.go:48: err :=
json.NewDecoder(r.Body).Decode(psm)
On 2016/06/09 18:00:55, dnj (Google) wrote:
> Why use JSON here and not protobuf?

this is just the pubsub message envelope thingy

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/pubsub.go:49: if err != nil {
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: if err := json.NewDecoder...

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/registry.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:22: var regKey = "holds a Registry"
On 2016/06/09 18:00:56, dnj (Google) wrote:
> nit: "holds a DM Distributor Registry"

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:27: return c
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Perhaps this should panic? I think that this is clearly a bug if this happens.

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:95: r.finishExecutionImpl = i
On 2016/06/09 18:00:55, dnj (Google) wrote:
> This seems like it would need locking. But it's not part of the interface...

it was a bogus extra method

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:122: func getTrimmedAppID(c
context.Context) string {
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Maybe this should be built into gae/info?

yeah probably. Done: https://chromiumcodereview.appspot.com/2062613002/

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:128: // loadConfig loads the named
distributor configurtaion from luci-config,
On 2016/06/09 18:00:55, dnj (Google) wrote:
> nit: "configuration"

derp

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:132: cfgSvc := config.Get(c)
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Note: You allow a nil config service in "init()", so this could be nil.

was missing a fallthrough there, added.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/registry.go:179: scheme + "://" +
inf.DefaultVersionHostname() + "/",
On 2016/06/09 18:00:55, dnj (Google) wrote:
> I think you should use a net.URL here, then call its String() method.

better, changed Config to have a url.URL natively.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/task_description.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/task_description.go:56: ret := *t.payload
On 2016/06/09 18:00:56, dnj (Google) wrote:
> I think you should build "clone" into "Quest_Desc".

yeah that's a better idea. I was tired :P. Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/test_registry.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/test_registry.go:20: var _ Registry =
(*testRegistry)(nil)
On 2016/06/09 18:00:56, dnj (Google) wrote:
> nit: this isn't necessary, since NewTestingRegistry does this assertion.

I like putting this near every type definition so it's an explicit assertion in
case the implicit assertions elsewhere in the code change.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/front...
File appengine/cmd/dm/frontend/app.yaml (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/front...
appengine/cmd/dm/frontend/app.yaml:18: LUCI_DM_CONFIG_BASE_PATH:
/s/infra/infra/go/src/github.com/luci/luci-go/appengine/cmd/dm/integration_tests/cfgdir/current
On 2016/06/09 18:00:56, dnj (Google) wrote:
> hrm? Should probably not commit.

Oh. Yeah no :).

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
File appengine/cmd/dm/model/attempt.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
appengine/cmd/dm/model/attempt.go:30: a.Failed = 0
On 2016/06/09 18:00:56, dnj (Google) wrote:
> *a = AttemptRetryState{} !

mmmm tasty.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
appengine/cmd/dm/model/attempt.go:53: AbnormalFinish dm.AbnormalFinish
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Why not make these pointers?

because luci/gae doesn't allow embed-by-pointer in this context, since these
implement PLS on (*struct). The issue is that if it's nil then Load wouldn't
have a way to populate it.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
File appengine/cmd/dm/model/execution.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
appengine/cmd/dm/model/execution.go:85: Attempt:
datastore.Get(c).MakeKey("Attempt", e.AttemptID().DMEncoded()),
On 2016/06/09 18:00:56, dnj (Google) wrote:
> AttemptKeyFromID?

good catch, I tried to find these all but apparently missed some.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
appengine/cmd/dm/model/execution.go:130: // Revoke will null-out the Token and
Put this Execution to the datastore. This
On 2016/06/09 18:00:56, dnj (Google) wrote:
> nit: "nil-out" in above documentation, "null-out" here, I prefer "clear" :)

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
appengine/cmd/dm/model/execution.go:186: code = codes.Unauthenticated
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Does this make sense? Unauthenticated specifically means that the user failed
to
> supply authentication.

Yeah, basically: unless we specifically tagged this error with a grpc code,
treat all other errors as Unauthenticated.

edit: OOPS! Yeah we should be using PermissionDenied here.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
File appengine/cmd/dm/model/quest.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/model...
appengine/cmd/dm/model/quest.go:23: func NewQuest(c context.Context, desc
*dm.Quest_Desc) *Quest {
On 2016/06/09 18:00:56, dnj (Google) wrote:
> It might not be a bad idea to omit "c" here and supply "now" externally. At
> worst this causes the same amount of work, and at best it makes this function
> purely operations on data and allows "Now" to be called fewer times if the
> caller already gets it. Just a thought.

I think I like the context better: the fact that we extract the current time is
an implementation detail of model.Quest, not a function of the caller. If there
were multiple permissible values to pass for the time value, then I think it
would make sense, but in this case it does not. Requiring users to call
NewQuest(clock.Now(c), desc) feels wrong to me.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
File appengine/cmd/dm/mutate/activate_execution.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/activate_execution.go:16: // ActivateExecution
executions an execution, moving it from the
On 2016/06/09 18:00:56, dnj (Google) wrote:
> nit: executions an execution!

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
File appengine/cmd/dm/mutate/add_deps.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/add_deps.go:72: mergeQuestMap :=
map[string]*MergeQuest{}
On 2016/06/09 18:00:56, dnj (Google) wrote:
> nit: avoid an allocation if not needed by surrounding with "len(a.Quests) >
0".
> Can also allocate to len(a.Quests), no?

Done

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/add_deps.go:98: for _, mut := range mergeQuestMap {
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Any chance this will ever exceed maximum datastore transaction size? It looks
> like it's pretty unbounded ATM.

Possibly, but that would require a lot to hit (~depending on 500 new quests),
there's possible optimization we can do here as well to make this limit higher
as well, if we hit it. Added a todo.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
File appengine/cmd/dm/mutate/finish_execution.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/finish_execution.go:78: if a.State ==
dm.Attempt_EXECUTING && a.CurExecution == f.EID.Id && !e.State.Terminal() {
On 2016/06/09 18:00:56, dnj (Google) wrote:
> For indentation purposes, maybe invert this and return early?

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
File appengine/cmd/dm/mutate/schedule_execution.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/schedule_execution.go:66: distTok, e.TimeToStart,
e.TimeToRun, e.TimeToStop, err = dist.Run(
On 2016/06/09 18:00:56, dnj (Google) wrote:
> Since it's possible for the transaction to fail, it's also possible that this
> request will be made to the distributor several times. I suppose the idea is
> that if this is a problem, the distributor can deduplicate requests based on
the
> parameters here if it wants?
> 
> How much do we care whether the distributor token is unique? If multiple
> executions share the same token, do we finish all of them when we get a
> completion notification?

This is documented in the distributor package, but yes, if the distributor can
deduplicate, it should do so.

Since we regenerate the execution's token every time this is run, only one of
them will win.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
File appengine/cmd/dm/mutate/timeout_execution.go (right):

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/timeout_execution.go:81: }.Errorf(c, "Could not
MakeDistributor")
On 2016/06/09 18:00:57, dnj (Google) wrote:
> Does this mean that if we ever unregister a Distributor, all of its pending
> executions will never timeout because this Mutation cannot advance? Maybe we
> need explicit "ErrDistributorDoesntExistDude" handling for this case. Or,
> alternatively, count this towards our maximum timeout attempts.

Added todo: need to distinguish luci-config flake from distributor config DNE.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/timeout_execution.go:86: if err != nil || rslt == nil &&
t.TimeoutAttempt < maxTimeoutAttempts {
On 2016/06/09 18:00:57, dnj (Google) wrote:
> rslt will never be nil here. Do you mean realRslt?

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/timeout_execution.go:89: "task_result":     rslt,
On 2016/06/09 18:00:57, dnj (Google) wrote:
> (same here)

Done.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/timeout_execution.go:100: muts = append(muts, t)
On 2016/06/09 18:00:56, dnj (Google) wrote:
> I think you'll like new Tumble API for this one :)

Acknowledged.

https://codereview.chromium.org/1537883002/diff/440001/appengine/cmd/dm/mutat...
appengine/cmd/dm/mutate/timeout_execution.go:104: if err != nil {
On 2016/06/09 18:00:57, dnj (Google) wrote:
> Can this ever be hit? You handle "err != nil" above in an OR, and I don't see
it
> getting changed since then.

I was missing some parens above.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
File common/api/dm/service/v1/graph_data.proto (right):

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:27: FAILED = 0;
On 2016/06/09 18:00:57, dnj (Google) wrote:
> On 2016/06/08 02:54:24, iannucci wrote:
> > I'm not super sure how to deal with this one: this is a swarming concept
that
> > I've ported to DM. Basically if the distributor says that it's "failed",
then
> > that shows up here. However, the way I plan to implement things in recipe
> > engine, the recipe itself should never exit non-0 (unless shit->fan). So in
> > normal recipe-based operation, we shouldn't actually see this.
> > 
> > The recipe engine would instead actually encapsulate the type of failure in
> its
> > normal json result blob (precise format TBD). This way depend_on could make
> > intelligent decisions about retries and such. And, of course, if the user
> wants
> > to have more that one failure flag, or status code (enum), they can include
> that
> > in the job result as well.
> 
> I think things pre-recipe-engine can fail (e.g., bad CIPD/Isolate hash in
> description, disk full, etc.). This sort of thing is probably retryable (error
> on the side of caution).

Yeah I think this makes sense. For recipe quests we'll (eventually?) set the #
of retries under this failure condition to 0.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:68: CANCELLED = 4;
On 2016/06/09 18:00:57, dnj (Google) wrote:
> On 2016/06/08 02:54:24, iannucci wrote:
> > later when DM supports cancellation directly, this status will be used.
> 
> nit:
> In American English, the verb cancel is usually inflected canceled and
> canceling—with one l. This is not a rule, however, and exceptions are easily
> found. In varieties of English from outside the U.S., including Canadian,
> British, and Australian English, cancelled and cancelling are the preferred
> spellings.
> 
> We're in 'Murica, son.

Who said protos were American? THEY LIVE IN CYBERSPACE WHERE THE QUEEN'S ENGLISH
REIGNS SUPREME!!!

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:75: // Attempts: the last Execution
had a REJECTED Status.
On 2016/06/09 18:00:57, dnj (Google) wrote:
> Is this not retryable? If we're out of quota, we might not be later. Maybe we
> need a specific "out of quota" failure reason vs. "your job is bad and you
> should feel bad."

I think this should definitely be retried at a higher level; if you're out of
quota, there's way too much nuance about when/if you should retry than we can
express here. If you're out of quota because jobs are eating your quota, the
correct response is to kill all the jobs, figure out why you ran out, fix that
and/or raise quota. DM shouldn't retry this.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:83: // Attempts: the last Execution
had a REJECTED Status.
On 2016/06/09 18:00:57, dnj (Google) wrote:
> MISSING Status?

oops

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:183: // The Attempt is waiting to be
Executed
On 2016/06/09 18:00:57, dnj (Google) wrote:
> nit: period at end.

Done.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:187: EXECUTING = 1;
On 2016/06/09 18:00:57, dnj (Google) wrote:
> IMO: s/the current/its current/

Done.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:195: // The Attempt is in an abnormal
final state
On 2016/06/09 18:00:57, dnj (Google) wrote:
> nit: period at end.

Done.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data.proto:226: string persistent_state_result =
4;
On 2016/06/09 18:00:57, dnj (Google) wrote:
> Is "bytes" more appropriate? "string" implies UTF8, which (given
> transformations) is obviously possible, but maybe not the most appropriate
form.

but string is the only map-key/immutable thing in go :(

I'll make it bytes on the hope we get `const []byte` at some point :(

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
File common/api/dm/service/v1/graph_data_purge_timestamps.go (right):

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/graph_data_purge_timestamps.go:7: // TimestampPurger is
for testing: invoking this on a struct in this package
On 2016/06/09 18:00:57, dnj (Google) wrote:
> I assume you have a good reason for this, so just a comment:
> 
> In general, I'd prefer you not do this. Since we have a test clock, we can
> control timestamps easily, and differences in timestamp expectations can point
> out real problems.

There's so much parallelism going on that it's not worth it to capture the
timestamps in the integration tests. Using testclock in a non-parallel unittest
is fine (which is what I do).

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
File common/api/dm/service/v1/quest_desc.go (right):

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/quest_desc.go:5: package dm
On 2016/06/09 18:00:57, dnj (Google) wrote:
> *yawn*

hmmm....

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
File common/api/dm/service/v1/quest_desc_normalize.go (right):

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/quest_desc_normalize.go:65: return
base64.URLEncoding.EncodeToString(h[:])[:QuestIDLength]
On 2016/06/09 18:00:57, dnj (Google) wrote:
> Note that if we ever change QuestIDLength, data could be hard to reach. TBH
I'd
> prefer we stick with full SHA256.

? We do, we're just chopping off the extra, constant-length '=' padding at the
end.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
File common/api/dm/service/v1/service.proto (right):

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/service.proto:18: // exposes DM's Dependency Server
API.
On 2016/06/09 18:00:57, dnj (Google) wrote:
> Why no caps?

Acknowledged.

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
File common/api/dm/service/v1/walk_graph.proto (right):

https://codereview.chromium.org/1537883002/diff/440001/common/api/dm/service/...
common/api/dm/service/v1/walk_graph.proto:132: bool all = 128;
On 2016/06/09 18:00:57, dnj (Google) wrote:
> (I don't like this. If you ever add new fields that you don't want to "all",
> this could become weird. Plus, this runs the risk of lazy folk simply wasting
> resources. I don't think it's unreasonable to ask people to just set the
things
> they want to "true".)

Hm. Ok. I'll move it to a package level constant Message that can be copied into
place. This was mainly for dmtool's usage where the entire message was dumped
for logging/debugging reasons.

https://codereview.chromium.org/1537883002/diff/460001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/distributor.go (right):

https://codereview.chromium.org/1537883002/diff/460001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:31: //   * In the DM frontend,
import your new package implementation and run its
On 2016/06/13 23:56:41, martiniss wrote:
> Filename you need to do this in? Or is that too fragile.

no specific filename.

https://codereview.chromium.org/1537883002/diff/460001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/distributor.go:58: // If this is !nil, the
Exectuion is counted as ABNORMAL_FINISHED, with this
On 2016/06/13 23:56:41, martiniss wrote:
> typo

Done.

[iannucci, 2016-06-15 00:46:21]

Description was changed from

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple, and EnsureGraphData
and WalkGraph have standardized on GraphData as their output format.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.
  * an 'Include.All' option was added to the WalkGraph options.

BUG=550684
==========

to

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple, and EnsureGraphData
and WalkGraph have standardized on GraphData as their output format.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.

BUG=550684
==========

[dnj (Google), 2016-06-16 16:57:22]

lgtm w/ comments/nits

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
File common/api/dm/service/v1/graph_data.proto (right):

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:68: CANCELLED = 4;
On 2016/06/15 00:46:01, iannucci wrote:
> On 2016/06/09 18:00:57, dnj (Google) wrote:
> > On 2016/06/08 02:54:24, iannucci wrote:
> > > later when DM supports cancellation directly, this status will be used.
> > 
> > nit:
> > In American English, the verb cancel is usually inflected canceled and
> > canceling—with one l. This is not a rule, however, and exceptions are easily
> > found. In varieties of English from outside the U.S., including Canadian,
> > British, and Australian English, cancelled and cancelling are the preferred
> > spellings.
> > 
> > We're in 'Murica, son.
> 
> Who said protos were American? THEY LIVE IN CYBERSPACE WHERE THE QUEEN'S
ENGLISH
> REIGNS SUPREME!!!

I'ma tell Obama.

https://chromiumcodereview.appspot.com/1537883002/diff/440001/common/api/dm/s...
common/api/dm/service/v1/graph_data.proto:226: string persistent_state_result =
4;
On 2016/06/15 00:46:02, iannucci wrote:
> On 2016/06/09 18:00:57, dnj (Google) wrote:
> > Is "bytes" more appropriate? "string" implies UTF8, which (given
> > transformations) is obviously possible, but maybe not the most appropriate
> form.
> 
> but string is the only map-key/immutable thing in go :(
> 
> I'll make it bytes on the hope we get `const []byte` at some point :(

Yeah I hear 'ya. But I think you did the right thing, b/c this is Protobuf, not
Go-tobuf.

https://chromiumcodereview.appspot.com/1537883002/diff/480001/appengine/cmd/d...
File appengine/cmd/dm/distributor/task_description.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/480001/appengine/cmd/d...
appengine/cmd/dm/distributor/task_description.go:43: topic =
pubsub.NewTopic(info.Get(t.c).TrimmedAppID(), notifyTopicSuffix)
It's not a bad idea to validate the topic here just to assert early if, for some
reason, it's bad (e.g., AppID is trash, notifyTopicSuffix is trash).

if err := topic.Validate(); err != nil {
  panic(fmt.Errorf("failed to validate Topic %q: %s", topic, err))
}

https://chromiumcodereview.appspot.com/1537883002/diff/480001/common/api/dm/s...
File common/api/dm/service/v1/graph_data_clone.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/480001/common/api/dm/s...
common/api/dm/service/v1/graph_data_clone.go:7: // Clone returns a deep copy of
this Quest_Desc.
nit: mention that this only works on Normalized descriptors? b/c if Meta or
Retry are nil, this will panic. Or is that implicit?

https://chromiumcodereview.appspot.com/1537883002/diff/480001/common/api/dm/s...
File common/api/dm/service/v1/walk_graph_normalize.go (right):

https://chromiumcodereview.appspot.com/1537883002/diff/480001/common/api/dm/s...
common/api/dm/service/v1/walk_graph_normalize.go:30: true, true, true, true,
true, math.MaxUint32, true, true, true,
nit: For this many fields, I think you should key:value assign, (a) for
readability, and (b) to protect against adding a boolean somewhere in the middle
of the struct later.

https://chromiumcodereview.appspot.com/1537883002/diff/480001/common/gcloud/p...
File common/gcloud/pubsub/topic.go (left):

https://chromiumcodereview.appspot.com/1537883002/diff/480001/common/gcloud/p...
common/gcloud/pubsub/topic.go:16: // NewTopic generates a new Subscritpion for a
given project and topic name.
I like how this was the wrong word, and it was also not spelled correctly.

[iannucci, 2016-06-18 01:35:41]

I'm now actually thinking that PersistentState should be a json object instead
of an opaque blob anyway:
  * it's actually part of the result, and should be indexed as such; being able
to search for it would be annoying (e.g. what job(s) produced hash X?)
  * distributors like swarming can trivially wrap their current state
    with a small doc to be compatible. It would take 6 extra bytes.
  * allows distributors to extend their state format without throwing off
    clients that currently read it.

I've made a note do do this in a later CL

https://codereview.chromium.org/1537883002/diff/480001/appengine/cmd/dm/distr...
File appengine/cmd/dm/distributor/task_description.go (right):

https://codereview.chromium.org/1537883002/diff/480001/appengine/cmd/dm/distr...
appengine/cmd/dm/distributor/task_description.go:43: topic =
pubsub.NewTopic(info.Get(t.c).TrimmedAppID(), notifyTopicSuffix)
On 2016/06/16 16:57:22, dnj (Google) wrote:
> It's not a bad idea to validate the topic here just to assert early if, for
some
> reason, it's bad (e.g., AppID is trash, notifyTopicSuffix is trash).
> 
> if err := topic.Validate(); err != nil {
>   panic(fmt.Errorf("failed to validate Topic %q: %s", topic, err))
> }

Done.

https://codereview.chromium.org/1537883002/diff/480001/common/api/dm/service/...
File common/api/dm/service/v1/graph_data_clone.go (right):

https://codereview.chromium.org/1537883002/diff/480001/common/api/dm/service/...
common/api/dm/service/v1/graph_data_clone.go:7: // Clone returns a deep copy of
this Quest_Desc.
On 2016/06/16 16:57:22, dnj (Google) wrote:
> nit: mention that this only works on Normalized descriptors? b/c if Meta or
> Retry are nil, this will panic. Or is that implicit?

Switched to proto.Clone anyway :)

https://codereview.chromium.org/1537883002/diff/480001/common/api/dm/service/...
File common/api/dm/service/v1/walk_graph_normalize.go (right):

https://codereview.chromium.org/1537883002/diff/480001/common/api/dm/service/...
common/api/dm/service/v1/walk_graph_normalize.go:30: true, true, true, true,
true, math.MaxUint32, true, true, true,
On 2016/06/16 16:57:22, dnj (Google) wrote:
> nit: For this many fields, I think you should key:value assign, (a) for
> readability, and (b) to protect against adding a boolean somewhere in the
middle
> of the struct later.

I'm intentionally not using fields so that if I add a new one, this becomes a
compilation error. If I use keys, then it will silently compile and be wrong :/

https://codereview.chromium.org/1537883002/diff/480001/common/gcloud/pubsub/t...
File common/gcloud/pubsub/topic.go (left):

https://codereview.chromium.org/1537883002/diff/480001/common/gcloud/pubsub/t...
common/gcloud/pubsub/topic.go:16: // NewTopic generates a new Subscritpion for a
given project and topic name.
On 2016/06/16 16:57:22, dnj (Google) wrote:
> I like how this was the wrong word, and it was also not spelled correctly.

:D

[iannucci, 2016-06-18 01:36:00]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-06-18 01:36:15]

The CQ bit was unchecked by iannucci@chromium.org

[commit-bot: I haz the power, 2016-06-18 01:36:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537883002/480001

[commit-bot: I haz the power, 2016-06-18 01:38:30]

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

[commit-bot: I haz the power, 2016-06-18 01:38:32]

Try jobs failed on following builders:
  Luci-go Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Preci...)
  Luci-go Mac Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Mac%20Tester/...)
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[iannucci, 2016-06-18 01:39:36]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-06-18 01:39:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@google.com
Link to the patchset: https://codereview.chromium.org/1537883002/#ps500001
(title: "fix last comment and tests")

[commit-bot: I haz the power, 2016-06-18 01:39:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537883002/500001

[commit-bot: I haz the power, 2016-06-18 01:54:53]

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

[commit-bot: I haz the power, 2016-06-18 01:54:55]

Try jobs failed on following builders:
  Luci-go Win Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Win%20Tester/...)

[iannucci, 2016-06-18 02:08:17]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-06-18 02:08:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@google.com
Link to the patchset: https://codereview.chromium.org/1537883002/#ps520001
(title: "fix imports and make dummy.go a real file")

[commit-bot: I haz the power, 2016-06-18 02:08:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537883002/520001

[commit-bot: I haz the power, 2016-06-18 02:19:29]

Description was changed from

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple, and EnsureGraphData
and WalkGraph have standardized on GraphData as their output format.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.

BUG=550684
==========

to

==========
Initial distributor implementation.

This adds the following:
  * A new 'distributor' subpackage which defines the internal interface between
DM and all distributor implementations (swarming, jobsim, etc.)
  * Scheduling logic in mutate; executions now get scheduled with distributors,
distributors can ping back about the status of executions.
  * DM supports quest-controllable DM-side timeouts for interactions with the
distributor. 
  * The pRPC interface has been reduced to 4 rpcs for full functionality. Of
those, ActivateExecution and FinishAttempt are very simple, and EnsureGraphData
and WalkGraph have standardized on GraphData as their output format.
  * The persistent-state API has been implemented as part of the distributor
interface.
  * The state machine flow has been simplified (elimination of 'adding deps'
state, attempts now are in the executing state for the full duration of an
outstanding execution, all failure states between attempts and executions are
unified and separate from a successful finished state).

Other misc changes:
  * "as_account" is part of the quest description
  * quests can be generated from templates as part of EnsureQuestData
  * a fake distributor implementation for testing
  * tests have been refactored to take advantage of fake distributor; the deps
tests were getting really annoying to update and keep correct.

BUG=550684

Committed:
https://github.com/luci/luci-go/commit/00e902769955ef55f947ae046b1b530f5498241e
==========

[commit-bot: I haz the power, 2016-06-18 02:19:31]

Committed patchset #27 (id:520001) as
https://github.com/luci/luci-go/commit/00e902769955ef55f947ae046b1b530f5498241e