LogDog: Fix archival Get/Tail implementations.

LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for sparse
indexes. Consequently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

TBR=nodir@chromium.org
BUG=chromium:654880
TEST=unit

Committed: https://github.com/luci/luci-go/commit/d65c2b2a5ebadb3ebfa0383630bbc1fa2b755a70

[dnj, 2016-10-19 23:16:43]

Patchset #1 (id:1) has been deleted

[dnj, 2016-10-19 23:18:59]

https://codereview.chromium.org/2435883002/diff/20001/common/iotools/counting...
File common/iotools/countingreader.go (right):

https://codereview.chromium.org/2435883002/diff/20001/common/iotools/counting...
common/iotools/countingreader.go:15: Count int64
This is trivial enough that I don't mind combining it. No reason not to offer
direct access to this, and I use it below to clear the count periodically.

https://codereview.chromium.org/2435883002/diff/20001/logdog/appengine/coordi...
File logdog/appengine/coordinator/endpoints/logs/get.go (left):

https://codereview.chromium.org/2435883002/diff/20001/logdog/appengine/coordi...
logdog/appengine/coordinator/endpoints/logs/get.go:172: MaxBytes:  byteLimit,
Getting rid of this is no big deal, since it's enforced in this code anyway.
This was a misguided attempt to not have GS fetch too much, but GS reader
fetches in chunks anyway, so we're fine.

https://codereview.chromium.org/2435883002/diff/20001/logdog/common/storage/a...
File logdog/common/storage/archive/storage.go (right):

https://codereview.chromium.org/2435883002/diff/20001/logdog/common/storage/a...
logdog/common/storage/archive/storage.go:1: // Copyright 2015 The LUCI Authors.
All rights reserved.
This file changed bigly.

[dnj, 2016-10-20 18:40:11]

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

[dnj, 2016-10-20 18:40:12]

PTAL. Just throwing this out b/c I have to run for the day, LMK if you have time
to review this and if you have any questions.

[dnj, 2016-10-26 17:33:50]

Description was changed from

==========
LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for spare
indexes. Conseqently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

BUG=chromium:654880
TEST=unit
==========

to

==========
LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for spare
indexes. Conseqently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

TBR=nodir@chromium.org
BUG=chromium:654880
TEST=unit
==========

[dnj, 2016-10-26 17:34:09]

I don't think anyone actually cares about this CL set, and the tests show that
it works, so I'm just going to TBR it.

[dnj, 2016-10-26 17:34:13]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-10-26 17:34:17]

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, 2016-10-26 18:20:51]

Description was changed from

==========
LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for spare
indexes. Conseqently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

TBR=nodir@chromium.org
BUG=chromium:654880
TEST=unit
==========

to

==========
LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for spare
indexes. Conseqently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

TBR=nodir@chromium.org
BUG=chromium:654880
TEST=unit

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

[commit-bot: I haz the power, 2016-10-26 18:20:52]

Committed patchset #1 (id:20001) as
https://github.com/luci/luci-go/commit/d65c2b2a5ebadb3ebfa0383630bbc1fa2b755a70

[nodir, 2016-10-26 19:58:39]

Description was changed from

==========
LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for spare
indexes. Conseqently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

TBR=nodir@chromium.org
BUG=chromium:654880
TEST=unit

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

to

==========
LogDog: Fix archival Get/Tail implementations.

The sparse index tail algorithm assumed that indexes always had an entry
for the last log in the index. This is not necessarily true for sparse
indexes. Consequently, archive Tail could return a non-tail entry in that
situation.

This also fixes the Get and Tail to be more robust against malformed index
and stream data files, to the point where it will happily operate
without an index if needed now.

However, this is actually a good idea, so update index generation to
always include a final entry to the index. Tail optimizes for this case.

This introduced yet another situation where the storage layer was
unmarshalling LogEntry protobufs, then passing the binary data up to the
higher level to unmarshal again! To prevent this from introducing
performance regressions, we now have the storage layer kick up a lazy
unmarshal-at-most-once struct instead of raw data. This will let higher
levels take advantage of storage layers that have to unmarshal
protobufs.

Finally, log archival fetching had no supporting unit tests. Write some,
with very high coverage.

TBR=nodir@chromium.org
BUG=chromium:654880
TEST=unit

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

[nodir, 2016-10-26 20:25:33]

https://codereview.chromium.org/2435883002/diff/20001/logdog/appengine/coordi...
File logdog/appengine/coordinator/endpoints/logs/get.go (right):

https://codereview.chromium.org/2435883002/diff/20001/logdog/appengine/coordi...
logdog/appengine/coordinator/endpoints/logs/get.go:237: byteLimit -= len(e.D)
you are mutating byteLimit in a retry loop. I think you have to copy byteLimit
at each attempt beginning

https://codereview.chromium.org/2435883002/diff/20001/logdog/appengine/coordi...
logdog/appengine/coordinator/endpoints/logs/get.go:245: count++
same here

[nodir, 2016-10-26 21:46:42]

lgtm