[tq] Enable task deletion.

[tq] Enable task deletion.

Enable named task deletion. This generalizes the task queue batching
function.

Add the concept of a name suffix to the task. This allows the user to
supply information without discarding sharding utility.

BUG=chromium:751925
TEST=unit

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

[dnj, 2017-08-03 17:45:07]

dnj@chromium.org changed reviewers:
+ tandrii@chromium.org, vadimsh@chromium.org

[dnj, 2017-08-03 17:45:07]

PTAL. This is needed by LogDog, and also a pretty nice addition.

[dnj, 2017-08-03 18:36:43]

reorder fields

[Vadim Sh., 2017-08-03 20:12:41]

lgtm

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go
File appengine/tq/tq.go (right):

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:134: if task.Payload != nil {
tasks without payload are meaningless in this model, since they are not
routable. Is this code path going to be used?

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:142: return strings.Join(parts, "")
nit: let's use '-' as separator (if it's allowed in task names...)

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:208: // containing only the non-nil errors. This simplifies
user expectations.
I did it this way because I had never seen we actually deconstruct MultiErrors
into per-component ones. And keeping order of errors when doing parallel ops is
a chore.

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:286: return errors.Filter(err, taskqueue.ErrTaskAlreadyAdded)
does this handle the case when err is taskqueue.ErrTaskAlreadyAdded (i.e it's a
singular error)?

[dnj, 2017-08-03 20:56:28]

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go
File appengine/tq/tq.go (right):

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:134: if task.Payload != nil {
On 2017/08/03 20:12:41, Vadim Sh. wrote:
> tasks without payload are meaningless in this model, since they are not
> routable. Is this code path going to be used?

No, I suppose a panic is appropriate.

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:142: return strings.Join(parts, "")
On 2017/08/03 20:12:41, Vadim Sh. wrote:
> nit: let's use '-' as separator (if it's allowed in task names...)

It is allowed. OK.

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:208: // containing only the non-nil errors. This simplifies
user expectations.
On 2017/08/03 20:12:41, Vadim Sh. wrote:
> I did it this way because I had never seen we actually deconstruct MultiErrors
> into per-component ones. And keeping order of errors when doing parallel ops
is
> a chore.

I think it's fine. If a user needs something more, can talk then. Did you want
me to update the comment or anything, or were you just offering some insight? :)

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:286: return errors.Filter(err, taskqueue.ErrTaskAlreadyAdded)
On 2017/08/03 20:12:41, Vadim Sh. wrote:
> does this handle the case when err is taskqueue.ErrTaskAlreadyAdded (i.e it's
a
> singular error)?

yep

[dnj, 2017-08-03 20:56:56]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-08-03 20:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2986373002/#ps80001
(title: "comments")

[commit-bot: I haz the power, 2017-08-03 20:57:04]

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-08-03 21:03:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1501793816452980,
"parent_rev": "a2c4aedab81206b6d01b85dae474f93ce9ed804c", "commit_rev":
"da9d004ddcf295086e66df450f6779839259be9c"}

[commit-bot: I haz the power, 2017-08-03 21:03:20]

Description was changed from

==========
[tq] Enable task deletion.

Enable named task deletion. This generalizes the task queue batching
function.

Add the concept of a name suffix to the task. This allows the user to
supply information without discarding sharding utility.

BUG=chromium:751925
TEST=unit
==========

to

==========
[tq] Enable task deletion.

Enable named task deletion. This generalizes the task queue batching
function.

Add the concept of a name suffix to the task. This allows the user to
supply information without discarding sharding utility.

BUG=chromium:751925
TEST=unit

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

[commit-bot: I haz the power, 2017-08-03 21:03:21]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/da9d004ddcf295086e66df450f6779839259be9c

[Vadim Sh., 2017-08-03 21:04:05]

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go
File appengine/tq/tq.go (right):

https://codereview.chromium.org/2986373002/diff/60001/appengine/tq/tq.go#newc...
appengine/tq/tq.go:208: // containing only the non-nil errors. This simplifies
user expectations.
On 2017/08/03 20:56:27, dnj wrote:
> On 2017/08/03 20:12:41, Vadim Sh. wrote:
> > I did it this way because I had never seen we actually deconstruct
MultiErrors
> > into per-component ones. And keeping order of errors when doing parallel ops
> is
> > a chore.
> 
> I think it's fine. If a user needs something more, can talk then. Did you want
> me to update the comment or anything, or were you just offering some insight?
:)

I'm just explaining why the code wasn't rigorous here initially.