logdog: Add bundler library.

logdog: Add bundler library.

BUG=chromium:550673
Committed: https://github.com/luci/luci-go/commit/338c0ee88fa8bf5f0879266889b7153a90067558

[dnj, 2015-11-03 19:04:57]

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

[dnj, 2015-11-03 19:04:57]

PTAL. This replaces the old Bundler (https://codereview.chromium.org/1429993002)
CL.

This is basically the heart of the Butler. Its job is to pull stream data and
compose it into Bundles.

[dnj, 2015-11-03 19:05:52]

On 2015/11/03 19:04:57, dnj wrote:
> PTAL. This replaces the old Bundler
(https://codereview.chromium.org/1429993002)
> CL.
> 
> This is basically the heart of the Butler. Its job is to pull stream data and
> compose it into Bundles.

(Old Bundler is actually: https://codereview.chromium.org/1276923003/)

[dnj, 2015-11-03 20:37:45]

Review order:
(butlerproto) https://codereview.chromium.org/1321273002/
(StreamServer) https://codereview.chromium.org/1429993002/
(Bundler) https://codereview.chromium.org/1412063008/
(Output) https://codereview.chromium.org/1211053004/

[estaab, 2015-11-12 00:51:48]

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
File client/internal/logdog/butler/bundler/parser_test.go (right):

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
client/internal/logdog/butler/bundler/parser_test.go:39: func
shouldMatchLogEntry(actual interface{}, expected ...interface{}) string {
I see a bunch of test helper code but not any actual tests. What am I missing?

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
File client/internal/logdog/butler/bundler/sizer.go (right):

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
client/internal/logdog/butler/bundler/sizer.go:4: 
Can you add some file-level comments describing the purpose of this code?

Actually, if doc.go had a lot more info that would be good also. I'd like to
know the public boundaries for this code, and a rough description of how these
files fit together to form the implementation.

I kind of want to hold off on reviewing the rest until I have a better
high-level mental model.

Thoughts?

[dnj (Google), 2015-11-12 02:00:40]

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

[dnj (Google), 2015-11-12 02:00:41]

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
File client/internal/logdog/butler/bundler/parser_test.go (right):

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
client/internal/logdog/butler/bundler/parser_test.go:39: func
shouldMatchLogEntry(actual interface{}, expected ...interface{}) string {
On 2015/11/12 00:51:48, estaab wrote:
> I see a bunch of test helper code but not any actual tests. What am I missing?

This helper code is used in *Parser_test.go tests.

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
File client/internal/logdog/butler/bundler/sizer.go (right):

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
client/internal/logdog/butler/bundler/sizer.go:4: 
On 2015/11/12 00:51:48, estaab wrote:
> Can you add some file-level comments describing the purpose of this code?
> 
> Actually, if doc.go had a lot more info that would be good also. I'd like to
> know the public boundaries for this code, and a rough description of how these
> files fit together to form the implementation.
> 
> I kind of want to hold off on reviewing the rest until I have a better
> high-level mental model.
> 
> Thoughts?

Done.

[estaab, 2015-11-13 04:02:47]

estaab@chromium.org changed reviewers:
+ martiniss@chromium.org

[estaab, 2015-11-13 04:02:47]

A few superficial comments. I looked over the change and struggled to come up
with anything of substance and I don't want to nitpick.

Stephen said he's willing to have a look at the change as well, it's probably
good to spread the reviews around more where we can.

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
File client/internal/logdog/butler/bundler/parser_test.go (right):

https://codereview.chromium.org/1412063008/diff/20001/client/internal/logdog/...
client/internal/logdog/butler/bundler/parser_test.go:39: func
shouldMatchLogEntry(actual interface{}, expected ...interface{}) string {
On 2015/11/12 02:00:41, dnj (Google) wrote:
> On 2015/11/12 00:51:48, estaab wrote:
> > I see a bunch of test helper code but not any actual tests. What am I
missing?
> 
> This helper code is used in *Parser_test.go tests.

Got it. I chatted with Robbie about names here and you might want to call this
something like parserTestLib_test.go or something that implies there aren't
tests for parser.go. I don't care too much after learning about go's naming
limitations around testing files, though.

https://codereview.chromium.org/1412063008/diff/40001/client/internal/logdog/...
File client/internal/logdog/butler/bundler/doc.go (right):

https://codereview.chromium.org/1412063008/diff/40001/client/internal/logdog/...
client/internal/logdog/butler/bundler/doc.go:9: // elastic set of Stream
instances, each of which containts state for a single
contains

https://codereview.chromium.org/1412063008/diff/40001/client/internal/logdog/...
client/internal/logdog/butler/bundler/doc.go:16: // full, the Stream will block
on appending data, whih will, in turn, block its
which

[iannucci, 2015-11-13 07:16:37]

iannucci@chromium.org changed reviewers:
- martiniss@chromium.org

[iannucci, 2015-11-13 07:16:38]

lgtm

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/binaryParser.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/binaryParser.go:25: func (p *binaryParser)
nextEntry(c *constraints) (*protocol.LogEntry, error) {
why return an error at all? this looks like it can only return nil or panic?

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/builder.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/builder.go:22: templateCachedSize int
this should go up near template. I thought it was a cached size of beMap.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/builder.go:24: beMap
map[string]*protocol.ButlerLogBundle_Entry
comment what the 'string' is?

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/bundler.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/bundler.go:116: }
can we do this before the lock? actually... can't we do all of this before the
lock and then just put the check above in registerStreamLocked?

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/bundler.go:277: // Performs a round of
bundling with a stream state. polling stream state and adding
this seems incomplete?

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/bundler.go:282: //     immediately.
are thresholds calculated absolutely, or from the last-sent bundle? e.g. can we
be 'behind' for multiple iterations in a row, or will sending a bundle always
reset the threshold?

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/datagramParser.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/datagramParser.go:15: // datagramParser is
a parser implementation for the LogDog DATAGRAb stream
DATAGRAB is actually the codename of a secret government program from the 90s.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/datagramParser.go:102: // truncate.
erm... does truncate actually mean truncate? I thought that these streams
allowed reconstruction of partials?

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/textParser.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/textParser.go:86: // next round.
what about weird stuff where the last bundle doesn't have space for the last
character... will it get it's own packet automatically, or is there an edge case
here?

[dnj, 2015-11-14 00:30:37]

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/binaryParser.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/binaryParser.go:25: func (p *binaryParser)
nextEntry(c *constraints) (*protocol.LogEntry, error) {
On 2015/11/13 07:16:38, iannucci wrote:
> why return an error at all? this looks like it can only return nil or panic?

This is implementing the "parser" interface, which allows for an error.

The error case is employed when the stream contains invalid data. This is only
used once, though, when a datagram's size prefix exceeds a limit. The intent is
to shut down the stream when this sort of thing happens.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/builder.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/builder.go:22: templateCachedSize int
On 2015/11/13 07:16:38, iannucci wrote:
> this should go up near template. I thought it was a cached size of beMap.

Done.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/builder.go:24: beMap
map[string]*protocol.ButlerLogBundle_Entry
On 2015/11/13 07:16:38, iannucci wrote:
> comment what the 'string' is?

Done.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/bundler.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/bundler.go:116: }
On 2015/11/13 07:16:38, iannucci wrote:
> can we do this before the lock? actually... can't we do all of this before the
> lock and then just put the check above in registerStreamLocked?

Yep, Done.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/bundler.go:277: // Performs a round of
bundling with a stream state. polling stream state and adding
On 2015/11/13 07:16:38, iannucci wrote:
> this seems incomplete?

Done.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/bundler.go:282: //     immediately.
On 2015/11/13 07:16:38, iannucci wrote:
> are thresholds calculated absolutely, or from the last-sent bundle? e.g. can
we
> be 'behind' for multiple iterations in a row, or will sending a bundle always
> reset the threshold?

They are relative to when the buffered data was ingested. I'll update the
comment.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/datagramParser.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/datagramParser.go:15: // datagramParser is
a parser implementation for the LogDog DATAGRAb stream
On 2015/11/13 07:16:38, iannucci wrote:
> DATAGRAB is actually the codename of a secret government program from the 90s.

A not-so-crafty code word!

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/datagramParser.go:102: // truncate.
On 2015/11/13 07:16:38, iannucci wrote:
> erm... does truncate actually mean truncate? I thought that these streams
> allowed reconstruction of partials?

Yeah "truncate" says, "you may truncate this data if you need to". In this case,
there is no data loss, but the data is allowed to be split.

For clarity's sake I'll update to "allowSplit".

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/doc.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/doc.go:9: // elastic set of Stream
instances, each of which containts state for a single
On 2015/11/13 04:02:47, estaab wrote:
> contains

Done.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/doc.go:16: // full, the Stream will block
on appending data, whih will, in turn, block its
On 2015/11/13 04:02:47, estaab wrote:
> which

Done.

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
File client/internal/logdog/butler/bundler/textParser.go (right):

https://chromiumcodereview.appspot.com/1412063008/diff/40001/client/internal/...
client/internal/logdog/butler/bundler/textParser.go:86: // next round.
On 2015/11/13 07:16:38, iannucci wrote:
> what about weird stuff where the last bundle doesn't have space for the last
> character... will it get it's own packet automatically, or is there an edge
case
> here?

"closed" means the input stream is closed. The bundler will still create as many
bundles as necessary to get all of the buffered data out.

[dnj, 2015-11-14 00:30:47]

Description was changed from

==========
logdog: Add bundler library.

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

to

==========
logdog: Add bundler library.

BUG=chromium:550673
==========

[dnj, 2015-11-19 19:16:05]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-11-19 19:16:06]

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

[commit-bot: I haz the power, 2015-11-19 19:16:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412063008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412063008/60001

[commit-bot: I haz the power, 2015-11-19 19:18:28]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/338c0ee88fa8bf5f0879266889b7153a90067558