[logdog] Replace Tumble with push queues.

[logdog] Replace Tumble with push queues.

Replace Tumble with push queues for archival.

Currently, LogDog uses the Tumble journaling state machine to manage its
archival. Tumble is generally overkill for this one-state task, but was
chosen because it seemed, at the time, likely that it would be used
everywhere in LUCI.

Almost two years later, LogDog is the only production major user of
Tumble. Since it barely scrapes the power of Tumble, and since Tumble
itself is rather opaque in its operations, this trade-off is not
worthwhile. Instead, we replace Tumble with task queues.

When a log stream is registered, an "expired" task will be enqueued to
handle it once the stream expires (if it never gets terminated). When
the stream is terminated, the expiration task is deleted, replaced with
a shorter-term archival task.

We leave Tumble and its mutation handling in-place because, in
production, there is still a Tumble backlog to process through. This
should be empited within a few days, and we can finish the removal.

BUG=chromium:751925
TEST=unit

Review-Url: https://codereview.chromium.org/2989333002
Committed: https://github.com/luci/luci-go/commit/18ae6bdbc6652e10610d55e4528d4bb3c624f813

[dnj, 2017-08-03 03:45:54]

dnj@chromium.org changed reviewers:
+ hinoka@chromium.org, iannucci@chromium.org

[dnj, 2017-08-03 03:45:55]

PTAL.

This CL makes me sad, but I think it's good for the long-term health of LogDog
:( OTOH, adding Tumble back in if it makes sense is also not horribly difficult.

https://codereview.chromium.org/2989333002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/backend/archival.go (right):

https://codereview.chromium.org/2989333002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/backend/archival.go:136: c := ctx.Context
Note: This logic is largely a clone of the Tumble mutation:

https://github.com/luci/luci-go/blob/master/logdog/appengine/coordinator/muta...

Notable differences:
1) We have to decode our HTTP request.
2) We use datastore RunInTransaction directly, instead of assuming we're already
in a transaction (which the Mutation does).

https://codereview.chromium.org/2989333002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/backend/routes.go (right):

https://codereview.chromium.org/2989333002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/backend/routes.go:15: package backend
This is actually a new file. I guess it thinks it's a copy of some random file.

https://codereview.chromium.org/2989333002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/coordinatorTest/context.go (right):

https://codereview.chromium.org/2989333002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/coordinatorTest/context.go:205: mainServicePath :=
filepath.Join("..", "..", "..", "cmd", "coordinator", "vmuser")
This is technically independent, but small enough that I don't really mind
heaping it in. Now that we're using an "index.yaml", we can directly test
against it instead of replicating it.

[Vadim Sh., 2017-08-03 06:10:16]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2017-08-03 06:10:16]

(you may or may not be interested in this thing:
https://godoc.org/github.com/luci/luci-go/scheduler/appengine/engine/tq It's
like python defer'ed but for Go, and using protos for serialization. It may be
an overkill if you have only one kind of task)

(if you are interested, we can move it to top level, it has no dependencies on
Scheduler stuff)

[dnj, 2017-08-03 15:43:22]

On 2017/08/03 06:10:16, Vadim Sh. wrote:
> (you may or may not be interested in this thing:
> https://godoc.org/github.com/luci/luci-go/scheduler/appengine/engine/tq It's
> like python defer'ed but for Go, and using protos for serialization. It may be
> an overkill if you have only one kind of task)
> 
> (if you are interested, we can move it to top level, it has no dependencies on
> Scheduler stuff)

That looks great, very lightweight. I filed:
https://codereview.chromium.org/2988413002

[Ryan Tseng, 2017-08-03 17:30:22]

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

[Ryan Tseng, 2017-08-03 17:30:23]

I'll let robbie look at this since he's more familiar with this, unless he's
super afk today, then I can look at it.

[dnj, 2017-08-03 18:38:08]

use name prefix instead of suffix

[dnj, 2017-08-03 18:41:12]

incorporate namespace in task name

[Ryan Tseng, 2017-08-03 19:30:05]

lgtm

didn't have a whole lot of comments, make sure you validate on -dev first.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
File logdog/appengine/coordinator/tasks/archival.go (right):

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:56: }.Errorf(c, "Failed to add
task to task queue.")
This will cause the same error to log multiple times, prefer
errors.Annotate(...) on the inside and log on the outside.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:68: func
ClearArchiveIncompleteExpiredTask(c context.Context, id coordinator.HashID)
error {
DeleteAllExpiredTasks()?

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:83: // handleArchiveDispatchTask
is a tq.Handler for an ArchiveDispatchTask.
What does it do?

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:117: log.WithError(err).Errorf(c,
"Failed to load archival log stream.")
Use errors.Annotate() instead for this transaction.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:161: // errCheckReader is an
io.Reader wrapper which tracks whether or not the
Is this used?

[iannucci, 2017-08-03 19:54:20]

lgtm

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
File logdog/api/endpoints/coordinator/services/v1/tasks.proto (right):

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/services/v1/tasks.proto:15: // The hash ID fo
the log stream to archive.
nit: s/fo/for

id is a `prefix/+/path`?

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/services/v1/tasks.proto:30:
google.protobuf.Duration settle_delay = 3;
why is this an aspect of the Task and not e.g. a setting in datastore?

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/services/v1/tasks.proto:38:
google.protobuf.Duration complete_period = 4;
same

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

use single publisher

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

named

[dnj, 2017-08-03 21:05:47]

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
File logdog/api/endpoints/coordinator/services/v1/tasks.proto (right):

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/services/v1/tasks.proto:15: // The hash ID fo
the log stream to archive.
On 2017/08/03 19:54:20, iannucci wrote:
> nit: s/fo/for
> 
> id is a `prefix/+/path`?

Nope, it's the hash of the full path. Within Coordinator code, this is well
understood, so shouldn't need elaboration here.

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/services/v1/tasks.proto:30:
google.protobuf.Duration settle_delay = 3;
On 2017/08/03 19:54:19, iannucci wrote:
> why is this an aspect of the Task and not e.g. a setting in datastore?

It starts as a "luci-config" setting, but is propagated along with the archival
parameters.

https://codereview.chromium.org/2989333002/diff/60001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/services/v1/tasks.proto:38:
google.protobuf.Duration complete_period = 4;
On 2017/08/03 19:54:19, iannucci wrote:
> same

(same)

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
File logdog/appengine/coordinator/tasks/archival.go (right):

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:56: }.Errorf(c, "Failed to add
task to task queue.")
On 2017/08/03 19:30:04, Ryan Tseng wrote:
> This will cause the same error to log multiple times, prefer
> errors.Annotate(...) on the inside and log on the outside.

Done.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:68: func
ClearArchiveIncompleteExpiredTask(c context.Context, id coordinator.HashID)
error {
On 2017/08/03 19:30:04, Ryan Tseng wrote:
> DeleteAllExpiredTasks()?

Nope, this clears a single archival tasks for the EXPIRED log stream state. I'll
change to: DeleteArchiveStreamExpiredTask.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:83: // handleArchiveDispatchTask
is a tq.Handler for an ArchiveDispatchTask.
On 2017/08/03 19:30:04, Ryan Tseng wrote:
> What does it do?

Updated comment.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:117: log.WithError(err).Errorf(c,
"Failed to load archival log stream.")
On 2017/08/03 19:30:04, Ryan Tseng wrote:
> Use errors.Annotate() instead for this transaction.

Done.

https://codereview.chromium.org/2989333002/diff/60001/logdog/appengine/coordi...
logdog/appengine/coordinator/tasks/archival.go:161: // errCheckReader is an
io.Reader wrapper which tracks whether or not the
On 2017/08/03 19:30:04, Ryan Tseng wrote:
> Is this used?

No, now that I'm using "tq". Is deleted.

[dnj, 2017-08-03 21:06:05]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-08-03 21:06:05]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com,
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2989333002/#ps100001
(title: "named")

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

CQ is trying da patch.

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

[dnj, 2017-08-03 21:07:31]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-08-03 21:07:32]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com,
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2989333002/#ps110001
(title: "comments")

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

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:14:19]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1501794451289970,
"parent_rev": "da9d004ddcf295086e66df450f6779839259be9c", "commit_rev":
"18ae6bdbc6652e10610d55e4528d4bb3c624f813"}

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

Description was changed from

==========
[logdog] Replace Tumble with push queues.

Replace Tumble with push queues for archival.

Currently, LogDog uses the Tumble journaling state machine to manage its
archival. Tumble is generally overkill for this one-state task, but was
chosen because it seemed, at the time, likely that it would be used
everywhere in LUCI.

Almost two years later, LogDog is the only production major user of
Tumble. Since it barely scrapes the power of Tumble, and since Tumble
itself is rather opaque in its operations, this trade-off is not
worthwhile. Instead, we replace Tumble with task queues.

When a log stream is registered, an "expired" task will be enqueued to
handle it once the stream expires (if it never gets terminated). When
the stream is terminated, the expiration task is deleted, replaced with
a shorter-term archival task.

We leave Tumble and its mutation handling in-place because, in
production, there is still a Tumble backlog to process through. This
should be empited within a few days, and we can finish the removal.

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

to

==========
[logdog] Replace Tumble with push queues.

Replace Tumble with push queues for archival.

Currently, LogDog uses the Tumble journaling state machine to manage its
archival. Tumble is generally overkill for this one-state task, but was
chosen because it seemed, at the time, likely that it would be used
everywhere in LUCI.

Almost two years later, LogDog is the only production major user of
Tumble. Since it barely scrapes the power of Tumble, and since Tumble
itself is rather opaque in its operations, this trade-off is not
worthwhile. Instead, we replace Tumble with task queues.

When a log stream is registered, an "expired" task will be enqueued to
handle it once the stream expires (if it never gets terminated). When
the stream is terminated, the expiration task is deleted, replaced with
a shorter-term archival task.

We leave Tumble and its mutation handling in-place because, in
production, there is still a Tumble backlog to process through. This
should be empited within a few days, and we can finish the removal.

BUG=chromium:751925
TEST=unit

Review-Url: https://codereview.chromium.org/2989333002
Committed:
https://github.com/luci/luci-go/commit/18ae6bdbc6652e10610d55e4528d4bb3c624f813
==========

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

Committed patchset #7 (id:110001) as
https://github.com/luci/luci-go/commit/18ae6bdbc6652e10610d55e4528d4bb3c624f813