LogDog: Handle archive failures.

LogDog: Handle archive failures.

The LogDog Coordinator now tracks failed stream archival attempts. If a
stream fails to archive, it will retry the archival task a specific
number of times, then ultimately accept the stream as archived.

Failed archival is only permitted when a log stream has had its
completeness requirement dropped.

This handles the case where a data corruption occurs. Currently, such a
log stream would stay in archival limbo forever, having incomplete
archival tasked but having the archival task fail each time. This allows
the stream to fail if it truly cannot be archived.

BUG=

[dnj, 2016-03-31 21:23:28]

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

[dnj, 2016-03-31 21:23:28]

PTAL

[dnj, 2016-03-31 22:13:41]

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

[dnj, 2016-03-31 22:13:41]

+vadimsh@, thanks!

[Vadim Sh., 2016-03-31 22:29:59]

lgtm with questions

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
File appengine/logdog/coordinator/endpoints/services/archiveStream.go (right):

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
appengine/logdog/coordinator/endpoints/services/archiveStream.go:102:
log.Fields{
maybe mark the entity with some indexable boolean, e.g. 

ls.ArchiveBroken = true

that way we can change maxArchiveErrors at any moment without thinking too hard
about the effect it may have

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
appengine/logdog/coordinator/endpoints/services/archiveStream.go:123: if err :=
ls.Put(di); err != nil {
this line got me confused :) usually it's di.Put(ls), no the other way around..

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
File appengine/logdog/coordinator/logStream.go (right):

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
appengine/logdog/coordinator/logStream.go:118: // If the value is <0:
typo: "If" is not needed here

https://codereview.chromium.org/1853433002/diff/1/server/internal/logdog/arch...
File server/internal/logdog/archivist/archivist.go (right):

https://codereview.chromium.org/1853433002/diff/1/server/internal/logdog/arch...
server/internal/logdog/archivist/archivist.go:130: task.ar.Error = true
are transient errors possible in "task.archive"? Is coordinator involved in
handling them (better to retry locally a bunch of times before bothering
coordinator).

[dnj, 2016-04-01 22:57:04]

Updated, PTAL. I added a formal "ArchiveState" constant that can be queried
against for streams in a specific state. The intent is ultimately to rewrite
"LogStream.State" queries in terms of ArchiveState and "_Terminated".

I'm not going to put off actually migrating until more serious changes (e.g.,
stuff for ACLs) are introduced unless I have a bout of free time.

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
File appengine/logdog/coordinator/endpoints/services/archiveStream.go (right):

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
appengine/logdog/coordinator/endpoints/services/archiveStream.go:102:
log.Fields{
On 2016/03/31 22:29:59, Vadim Sh. wrote:
> maybe mark the entity with some indexable boolean, e.g. 
> 
> ls.ArchiveBroken = true
> 
> that way we can change maxArchiveErrors at any moment without thinking too
hard
> about the effect it may have

Done. I introduced "ArchiveState", which can be either NOT, IS, PARTIAL, or
ERROR. I am intending to replace the log stream's "State" field with this, but
it will require a datastore migration, so I'm going to save that for something
more significant and dual-field for now.

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
appengine/logdog/coordinator/endpoints/services/archiveStream.go:123: if err :=
ls.Put(di); err != nil {
On 2016/03/31 22:29:59, Vadim Sh. wrote:
> this line got me confused :) usually it's di.Put(ls), no the other way
around..

Yeah LogStream.Put is the same as di.Put, but forces validation first :)

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
File appengine/logdog/coordinator/logStream.go (right):

https://codereview.chromium.org/1853433002/diff/1/appengine/logdog/coordinato...
appengine/logdog/coordinator/logStream.go:118: // If the value is <0:
On 2016/03/31 22:29:59, Vadim Sh. wrote:
> typo: "If" is not needed here

Acknowledged.

https://codereview.chromium.org/1853433002/diff/1/server/internal/logdog/arch...
File server/internal/logdog/archivist/archivist.go (right):

https://codereview.chromium.org/1853433002/diff/1/server/internal/logdog/arch...
server/internal/logdog/archivist/archivist.go:130: task.ar.Error = true
On 2016/03/31 22:29:59, Vadim Sh. wrote:
> are transient errors possible in "task.archive"? Is coordinator involved in
> handling them (better to retry locally a bunch of times before bothering
> coordinator).

They are retried in task.archive already as part of BigTable and GS shims.

[Vadim Sh., 2016-04-01 23:04:01]

https://codereview.chromium.org/1853433002/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/archiveStream.go (right):

https://codereview.chromium.org/1853433002/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/archiveStream.go:126: case
!req.Complete:
shouldn't this two cases be swapped?

If req.Error is true and req.Complete is false, we want to get ArchiveState ==
ArchivedPartially, since it's more severe condition compared to
ArchivedWithErrors (or not?).

[dnj, 2016-04-01 23:05:38]

https://codereview.chromium.org/1853433002/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/archiveStream.go (right):

https://codereview.chromium.org/1853433002/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/archiveStream.go:126: case
!req.Complete:
On 2016/04/01 23:04:01, Vadim Sh. wrote:
> shouldn't this two cases be swapped?
> 
> If req.Error is true and req.Complete is false, we want to get ArchiveState ==
> ArchivedPartially, since it's more severe condition compared to
> ArchivedWithErrors (or not?).

ArchivedWithErrors is the most severe, as it means the archival straight up
didn't work (e.g., data corruption). !Complete means that everything was great,
but some logs were missing.

[Vadim Sh., 2016-04-01 23:09:19]

lgtm

On 2016/04/01 23:05:38, dnj wrote:
>
https://codereview.chromium.org/1853433002/diff/40001/appengine/logdog/coordi...
> File appengine/logdog/coordinator/endpoints/services/archiveStream.go (right):
> 
>
https://codereview.chromium.org/1853433002/diff/40001/appengine/logdog/coordi...
> appengine/logdog/coordinator/endpoints/services/archiveStream.go:126: case
> !req.Complete:
> On 2016/04/01 23:04:01, Vadim Sh. wrote:
> > shouldn't this two cases be swapped?
> > 
> > If req.Error is true and req.Complete is false, we want to get ArchiveState
==
> > ArchivedPartially, since it's more severe condition compared to
> > ArchivedWithErrors (or not?).
> 
> ArchivedWithErrors is the most severe, as it means the archival straight up
> didn't work (e.g., data corruption). !Complete means that everything was
great,
> but some logs were missing.

Ok, I misread the enum comments then. I interpreted ArchivedWithErrors as "some
errors did happen, but we retried and succeeded, so everything is great".

[dnj, 2016-04-07 00:45:39]

Description was changed from

==========
LogDog: Handle archive failures.

The LogDog Coordinator now tracks failed stream archival attempts. If a
stream fails to archive, it will retry the archival task a specific
number of times, then ultimately accept the stream as archived.

Failed archival is only permitted when a log stream has had its
completeness requirement dropped.

This handles the case where a data corruption occurs. Currently, such a
log stream would stay in archival limbo forever, having incomplete
archival tasked but having the archival task fail each time. This allows
the stream to fail if it truly cannot be archived.

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

to

==========
LogDog: Handle archive failures.

The LogDog Coordinator now tracks failed stream archival attempts. If a
stream fails to archive, it will retry the archival task a specific
number of times, then ultimately accept the stream as archived.

Failed archival is only permitted when a log stream has had its
completeness requirement dropped.

This handles the case where a data corruption occurs. Currently, such a
log stream would stay in archival limbo forever, having incomplete
archival tasked but having the archival task fail each time. This allows
the stream to fail if it truly cannot be archived.

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