Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1276923003:
logdog: Add bundler library. (Closed)
Descriptionlogdog: Add bundler library.
BUG=
Patch Set 1 : #
Total comments: 51
Patch Set 2 : Addressed code review comments. #
Total comments: 9
Patch Set 3 : More documentation, simplified "bundlerStream" struct. #
Total comments: 13
Patch Set 4 : Rewrote bundle logic (and associated updates). #
Total comments: 18
Patch Set 5 : Updated from review. #
Messages
Total messages: 19 (9 generated)
dnj@chromium.org changed reviewers: + iannucci@chromium.org
PTAL
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
tandrii@chromium.org changed reviewers: + tandrii@chromium.org
haven't looked at tests yet. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:13: // Sizer is the Sizer instance to use to help with Bundler accounting. start comment with func name "// NewSizer https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:25: Size() int is int enough? maybe int64? https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:29: AppendBundleEntry(*protocol.ButlerLogBundle_Entry) this doesn't return https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:33: AppendLogEntry(*protocol.ButlerLogBundle_Entry, *protocol.LogEntry) this doesn't return https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:46: // Bundle converts the current set of buffered log data into a series of nit: s/Bundle/GetBundles https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:11: "github.com/luci/luci-go/common/logdog/protocol/protoutil" i think you have CL on which this one depends, but somehow I don't see it mentioned (2+weeks old "git cl upload" does it automatically). https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:15: type bundlerStream struct { this isn't used in this CL. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:38: func New(c Config) Bundler { Why pass config as a copy if you take a ref to it anyways? Given that Config is very small struct, i'd pass it as a copy everywhere. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:42: entries: make(map[types.StreamPath]*bundlerStream), nit: map[types.StreamPath]*bundlerStream{} https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:84: b.count = 0 shouldn't this also reset the entries? https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:110: bundle := *b.bundle i'd add comment here that you really want to copy bundle. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:111: bundle.Entries = append([]*protocol.ButlerLogBundle_Entry(nil), bundle.Entries...) I don't understand why you copy bundle entries here. On line #136 you append essentially a copy, possibly modified, of the entry which is already in this list. So, maybe you intended to just create new slice here? Also, why []*protocol.ButlerLogBundle_Entry(nil) and not []*protocol.ButlerLogBundle_Entry{} ? Is this to avoid extra slice memory allocation? Cryptic but neat replacement for ugly copy command :) https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:112: sizer := b.newSizer(&bundle) nit: i'd move this line closer to overThreshold func (line #120) https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:119: sort.Sort(sort.StringSlice(keys)) just sort.Strings(keys) https://golang.org/pkg/sort/#Strings https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:129: // Can we add this entry without exceeding our size threshold? what if threshold is small enough that the number of possible entries is 0? Given overestimation of fastSizer and I claim it could happen. Then, how do we ensure progress of our system? Do you argue that's super unlikely because threshold will always be at least say 1MB? https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:135: ec.Logs = append([]*protocol.LogEntry(nil), ec.Logs...) same as line #111: why copy ec.Logs here if you essentially append them one by one below? https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:142: e.logs = append(e.logs[:0], e.logs[i:]...) My test http://play.golang.org/p/g0387lE-R7 sort of says it works (see array c and how it modified a). Neat. However, append() doesn't guarantee that it moves elements one by one and in order, at least not in docs. So, maybe it's better to play safe and just allocate new memory? Or, if not allocate, then roll your own for loop. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/sizer_fast.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/sizer_fast.go:56: return varintLength(uint64(tag) << 3), nil I guess that's how protobuf encodes stuff. Still would be nice to have a url to the doc and maybe short explanation. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/sizer_fast.go:90: return -1 Special return values => who are you, a C programmer? :) This func should return (int, error) to be more Go-like and it'd be line-neutral change :) https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/sizer_fast.go:110: // NewFastSizer is a Sizer that is optimized for LogDog protobufs. nit: i prefer to put public stuff at the top
dnj@google.com changed reviewers: + dnj@google.com
Hey! Thanks for the thorough review. I've updated things to hopefully be both cleaner and more efficient. PTAL! https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:13: // Sizer is the Sizer instance to use to help with Bundler accounting. On 2015/08/11 14:06:07, tandrii(chromium) wrote: > start comment with func name "// NewSizer Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:25: Size() int On 2015/08/11 14:06:07, tandrii(chromium) wrote: > is int enough? maybe int64? Int should be enough, since in practice the size constraint is going to be in the 1-10MB range, but I'll bump to int64 for purity's sake. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:29: AppendBundleEntry(*protocol.ButlerLogBundle_Entry) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > this doesn't return It sure doesn't. Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:33: AppendLogEntry(*protocol.ButlerLogBundle_Entry, *protocol.LogEntry) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > this doesn't return Acknowledged. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:46: // Bundle converts the current set of buffered log data into a series of On 2015/08/11 14:06:07, tandrii(chromium) wrote: > nit: s/Bundle/GetBundles Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:11: "github.com/luci/luci-go/common/logdog/protocol/protoutil" On 2015/08/11 14:06:07, tandrii(chromium) wrote: > i think you have CL on which this one depends, but somehow I don't see it > mentioned (2+weeks old "git cl upload" does it automatically). https://codereview.chromium.org/1272893004 https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:15: type bundlerStream struct { On 2015/08/11 14:06:07, tandrii(chromium) wrote: > this isn't used in this CL. Hm? It's used all over the place in this file. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:38: func New(c Config) Bundler { On 2015/08/11 14:06:07, tandrii(chromium) wrote: > Why pass config as a copy if you take a ref to it anyways? Given that Config is > very small struct, i'd pass it as a copy everywhere. No point. This function owns this instance of Config, so might as well just ref it. It's a good pattern, moreso when Config is larger but here too. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:42: entries: make(map[types.StreamPath]*bundlerStream), On 2015/08/11 14:06:07, tandrii(chromium) wrote: > nit: map[types.StreamPath]*bundlerStream{} *sigh* Any idea why we seem to be shying away from perfectly normal initializers? Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:84: b.count = 0 On 2015/08/11 14:06:07, tandrii(chromium) wrote: > shouldn't this also reset the entries? The idea was that "GetBundles" will consume all bundles in "entries", implicitly clearing it. However, there is an edge case (threshold is too small to consume any bundle) that I wasn't taking in to account, so I'll clear it here. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:110: bundle := *b.bundle On 2015/08/11 14:06:07, tandrii(chromium) wrote: > i'd add comment here that you really want to copy bundle. Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:111: bundle.Entries = append([]*protocol.ButlerLogBundle_Entry(nil), bundle.Entries...) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > I don't understand why you copy bundle entries here. On line #136 you append > essentially a copy, possibly modified, of the entry which is already in this > list. So, maybe you intended to just create new slice here? > > Also, why []*protocol.ButlerLogBundle_Entry(nil) and not > []*protocol.ButlerLogBundle_Entry{} ? Is this to avoid extra slice memory > allocation? Originally it was to hedge the case where the template bundle had existing log entries. I was overthinking it, though. I've updated the code: template bundle/bundle-entry will never have logs, no need to accommodate that. > Cryptic but neat replacement for ugly copy command :) Heh thanks :) https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:112: sizer := b.newSizer(&bundle) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > nit: i'd move this line closer to overThreshold func (line #120) Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:119: sort.Sort(sort.StringSlice(keys)) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > just sort.Strings(keys) > https://golang.org/pkg/sort/#Strings Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:129: // Can we add this entry without exceeding our size threshold? On 2015/08/11 14:06:07, tandrii(chromium) wrote: > what if threshold is small enough that the number of possible entries is 0? > Given overestimation of fastSizer and I claim it could happen. Then, how do we > ensure progress of our system? > > Do you argue that's super unlikely because threshold will always be at least say > 1MB? It's allowed to happen. In that case, we will just drop log messages. I'll update comments/code to reflect this and add test cases. In practice it won't, because the single place that this is used will never have such a low constraint, but it would be a nasty surprise not to accommodate it. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:135: ec.Logs = append([]*protocol.LogEntry(nil), ec.Logs...) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > same as line #111: why copy ec.Logs here if you essentially append them one by > one below? Don't anymore; mandatory "nil" assumption. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:142: e.logs = append(e.logs[:0], e.logs[i:]...) On 2015/08/11 14:06:07, tandrii(chromium) wrote: > My test http://play.golang.org/p/g0387lE-R7 sort of says it works (see array c > and how it modified a). Neat. > > However, append() doesn't guarantee that it moves elements one by one and in > order, at least not in docs. So, maybe it's better to play safe and just > allocate new memory? Or, if not allocate, then roll your own for loop. Good thought! However, from here, it does say that append is allowed to overlap: http://golang.org/ref/spec#Appending_and_copying_slices https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/sizer_fast.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/sizer_fast.go:56: return varintLength(uint64(tag) << 3), nil On 2015/08/11 14:06:08, tandrii(chromium) wrote: > I guess that's how protobuf encodes stuff. Still would be nice to have a url to > the doc and maybe short explanation. Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/sizer_fast.go:90: return -1 On 2015/08/11 14:06:08, tandrii(chromium) wrote: > Special return values => who are you, a C programmer? :) > This func should return (int, error) to be more Go-like and it'd be line-neutral > change :) Done. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/sizer_fast.go:110: // NewFastSizer is a Sizer that is optimized for LogDog protobufs. On 2015/08/11 14:06:08, tandrii(chromium) wrote: > nit: i prefer to put public stuff at the top Done.
lgtm % 2 suggestions + 2 nits https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:15: type bundlerStream struct { On 2015/08/11 16:24:01, dnj (Google) wrote: > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > this isn't used in this CL. > > Hm? It's used all over the place in this file. sorry, that was meant for a different line, and even there it was wrong. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:38: func New(c Config) Bundler { On 2015/08/11 16:24:01, dnj (Google) wrote: > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > Why pass config as a copy if you take a ref to it anyways? Given that Config > is > > very small struct, i'd pass it as a copy everywhere. > > No point. This function owns this instance of Config, so might as well just ref > it. It's a good pattern, moreso when Config is larger but here too. sorry, my comment was stupid because I didn't quite capture that you embed Config into bundlerImpl. The right question is why not "func New(c *Config) Bundler {" https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:42: entries: make(map[types.StreamPath]*bundlerStream), On 2015/08/11 16:24:01, dnj (Google) wrote: > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > nit: map[types.StreamPath]*bundlerStream{} > > *sigh* Any idea why we seem to be shying away from perfectly normal > initializers? Done. hm, don't you still need an instance of bundle? It currently is nil. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:84: b.count = 0 On 2015/08/11 16:24:01, dnj (Google) wrote: > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > shouldn't this also reset the entries? > > The idea was that "GetBundles" will consume all bundles in "entries", implicitly > clearing it. However, there is an edge case (threshold is too small to consume > any bundle) that I wasn't taking in to account, so I'll clear it here. Acknowledged. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:142: e.logs = append(e.logs[:0], e.logs[i:]...) On 2015/08/11 16:24:01, dnj (Google) wrote: > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > My test http://play.golang.org/p/g0387lE-R7 sort of says it works (see array c > > and how it modified a). Neat. > > > > However, append() doesn't guarantee that it moves elements one by one and in > > order, at least not in docs. So, maybe it's better to play safe and just > > allocate new memory? Or, if not allocate, then roll your own for loop. > > Good thought! However, from here, it does say that append is allowed to > overlap: > http://golang.org/ref/spec#Appending_and_copying_slices Déjà vu: I knew I saw it somewhere, but yet again couldn't find it easily (who checks second link in Google search anyway?). Thanks! https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:5: package bundler I guess this CL introduces this package. If so, it'd be great to have doc.go :) https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:64: // unnecessary intermediate size computations. agree https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:148: } I think you need to check for (count == 0). And also, checks in line #135..137 aren't necessary then. https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_test.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_test.go:301: Convey(`When the base bundle is above threshold, clear logs and returns nil.`, func() { real nit: s/clear/clears here and on line #311. :)
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:38: func New(c Config) Bundler { On 2015/08/11 17:32:22, tandrii(chromium) wrote: > On 2015/08/11 16:24:01, dnj (Google) wrote: > > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > > Why pass config as a copy if you take a ref to it anyways? Given that Config > > is > > > very small struct, i'd pass it as a copy everywhere. > > > > No point. This function owns this instance of Config, so might as well just > ref > > it. It's a good pattern, moreso when Config is larger but here too. > > sorry, my comment was stupid because I didn't quite capture that you embed > Config into bundlerImpl. The right question is why not "func New(c *Config) > Bundler {" In the end it would have the same effect. However, by not using a reference in the function signature, it's very clear to the caller that the configuration will be copied. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:42: entries: make(map[types.StreamPath]*bundlerStream), On 2015/08/11 17:32:22, tandrii(chromium) wrote: > On 2015/08/11 16:24:01, dnj (Google) wrote: > > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > > nit: map[types.StreamPath]*bundlerStream{} > > > > *sigh* Any idea why we seem to be shying away from perfectly normal > > initializers? Done. > > hm, don't you still need an instance of bundle? It currently is nil. "bundle" has been replaced with "Config.TemplateBundle". https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:142: e.logs = append(e.logs[:0], e.logs[i:]...) On 2015/08/11 17:32:22, tandrii(chromium) wrote: > On 2015/08/11 16:24:01, dnj (Google) wrote: > > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > > My test http://play.golang.org/p/g0387lE-R7 sort of says it works (see array > c > > > and how it modified a). Neat. > > > > > > However, append() doesn't guarantee that it moves elements one by one and in > > > order, at least not in docs. So, maybe it's better to play safe and just > > > allocate new memory? Or, if not allocate, then roll your own for loop. > > > > Good thought! However, from here, it does say that append is allowed to > > overlap: > > http://golang.org/ref/spec#Appending_and_copying_slices > > Déjà vu: I knew I saw it somewhere, but yet again couldn't find it easily (who > checks second link in Google search anyway?). Thanks! No worries, it took me a bit of digging to find, so it was hardly obvious. Maybe Go should, you know, add this to the "append" function's documentation or something... https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler.go:5: package bundler On 2015/08/11 17:32:22, tandrii(chromium) wrote: > I guess this CL introduces this package. If so, it'd be great to have doc.go :) Done. https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:148: } On 2015/08/11 17:32:22, tandrii(chromium) wrote: > I think you need to check for (count == 0). And also, checks in line #135..137 > aren't necessary then. Ah I see what you're saying. Not correct, though. A ButlerLogBundle_Entry can have the following functions: 1) A ButlerLogBundle_Entry can export the log stream's "terminal" status (e.g., "yo Collector, this log stream is done, go ahead and archive it if you want."). This can be done without any log entries (count == 0). 2) A ButlerLogBundle_Entry can export log data via "entry.Logs". This is the count > 0 case. 3) (1) + (2) If count is 0, we still want to export the ButlerLogBundle_Entry if it's terminal. It must be either terminal or contain log data in order to be present in the "entries" map, since in Append we discard non-terminal empty bundle entries. I'll add a comment to this effect. https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_test.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_test.go:301: Convey(`When the base bundle is above threshold, clear logs and returns nil.`, func() { On 2015/08/11 17:32:22, tandrii(chromium) wrote: > real nit: s/clear/clears here and on line #311. :) Done.
lgtm https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:38: func New(c Config) Bundler { On 2015/08/11 18:14:27, dnj wrote: > On 2015/08/11 17:32:22, tandrii(chromium) wrote: > > On 2015/08/11 16:24:01, dnj (Google) wrote: > > > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > > > Why pass config as a copy if you take a ref to it anyways? Given that > Config > > > is > > > > very small struct, i'd pass it as a copy everywhere. > > > > > > No point. This function owns this instance of Config, so might as well just > > ref > > > it. It's a good pattern, moreso when Config is larger but here too. > > > > sorry, my comment was stupid because I didn't quite capture that you embed > > Config into bundlerImpl. The right question is why not "func New(c *Config) > > Bundler {" > > In the end it would have the same effect. However, by not using a reference in > the function signature, it's very clear to the caller that the configuration > will be copied. Acknowledged. https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:42: entries: make(map[types.StreamPath]*bundlerStream), On 2015/08/11 18:14:27, dnj wrote: > On 2015/08/11 17:32:22, tandrii(chromium) wrote: > > On 2015/08/11 16:24:01, dnj (Google) wrote: > > > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > > > nit: map[types.StreamPath]*bundlerStream{} > > > > > > *sigh* Any idea why we seem to be shying away from perfectly normal > > > initializers? Done. > > > > hm, don't you still need an instance of bundle? It currently is nil. > > "bundle" has been replaced with "Config.TemplateBundle". ah, not in PS2, though. In PS3 -> ok :) https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:148: } On 2015/08/11 18:14:28, dnj wrote: > On 2015/08/11 17:32:22, tandrii(chromium) wrote: > > I think you need to check for (count == 0). And also, checks in line #135..137 > > aren't necessary then. > > Ah I see what you're saying. Not correct, though. A ButlerLogBundle_Entry can > have the following functions: > 1) A ButlerLogBundle_Entry can export the log stream's "terminal" status (e.g., > "yo Collector, this log stream is done, go ahead and archive it if you want."). > This can be done without any log entries (count == 0). > 2) A ButlerLogBundle_Entry can export log data via "entry.Logs". This is the > count > 0 case. > 3) (1) + (2) > > If count is 0, we still want to export the ButlerLogBundle_Entry if it's > terminal. It must be either terminal or contain log data in order to be present > in the "entries" map, since in Append we discard non-terminal empty bundle > entries. > > I'll add a comment to this effect. OK, i take back that early check is not necessary. However, count may still be exactly 0 because of overThreshold() (line #151 in PS3 or #143 in this one). Then, if the the entry is not exporting terminal, you'd just add it to bundle, but it'd be useless (though not necessarily harmful either). https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/bundler_impl.go:54: logs := e.GetLogs() e.GetLogs() is cheap, and IMO, it's cleaner in this func to just refer to e.GetLogs() than to logs. https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/bundler_impl.go:75: cur = (*bundlerStream)(e) i'd merge this and following lines. https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/bundler_impl.go:144: if overThreshold() { ok, I agree this is necessary. https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... File client/internal/logdog/butler/bundler/doc.go (right): https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/doc.go:9: // Butler protobufs together for output. It primarily focused on two protobufs: nit: It *is* primarily... https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/doc.go:11: // metadata belongining to a single stream. s/belongining/belonging
Alright, I really hate to do this. However, your comment on the "count = 0" case got me thinking: Worst case I bundle a non-terminal message, but since I can't reverse the Sizer, I've already committed to it by the time I realize its logs won't fit. Gross. So I looked into how to fix it and ultimately realized two things: 1) I'd need to implement an Undo-based scheme in the sizer in order to properly test sizes and then back out if they're too big. 2) The "log is too big" worst-case for the previous packing algorithm was really gross. To the point where large LogEntry would cause a lot of data loss. I opted to reimplement the packing algorithm to be more efficient, both at runtime and at packing. This is reflected in the unit tests. I've had to change a few interfaces around, but I think it's overall better by far. There's more layers of craziness in "getBundle" now (I tried to comment), but it should be really fast in the common case (packing doesn't bump against threshold) and good at accommodating the other cases. "bundler_impl.go" has 100% coverage. PTAL! https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/40001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:42: entries: make(map[types.StreamPath]*bundlerStream), On 2015/08/11 18:31:05, tandrii(chromium) wrote: > On 2015/08/11 18:14:27, dnj wrote: > > On 2015/08/11 17:32:22, tandrii(chromium) wrote: > > > On 2015/08/11 16:24:01, dnj (Google) wrote: > > > > On 2015/08/11 14:06:07, tandrii(chromium) wrote: > > > > > nit: map[types.StreamPath]*bundlerStream{} > > > > > > > > *sigh* Any idea why we seem to be shying away from perfectly normal > > > > initializers? Done. > > > > > > hm, don't you still need an instance of bundle? It currently is nil. > > > > "bundle" has been replaced with "Config.TemplateBundle". > > ah, not in PS2, though. In PS3 -> ok :) Well, not used in PS2, properly removed in PS3 :) https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/60001/client/internal/logdog/... client/internal/logdog/butler/bundler/bundler_impl.go:148: } On 2015/08/11 18:31:05, tandrii(chromium) wrote: > On 2015/08/11 18:14:28, dnj wrote: > > On 2015/08/11 17:32:22, tandrii(chromium) wrote: > > > I think you need to check for (count == 0). And also, checks in line > #135..137 > > > aren't necessary then. > > > > Ah I see what you're saying. Not correct, though. A ButlerLogBundle_Entry can > > have the following functions: > > 1) A ButlerLogBundle_Entry can export the log stream's "terminal" status > (e.g., > > "yo Collector, this log stream is done, go ahead and archive it if you > want."). > > This can be done without any log entries (count == 0). > > 2) A ButlerLogBundle_Entry can export log data via "entry.Logs". This is the > > count > 0 case. > > 3) (1) + (2) > > > > If count is 0, we still want to export the ButlerLogBundle_Entry if it's > > terminal. It must be either terminal or contain log data in order to be > present > > in the "entries" map, since in Append we discard non-terminal empty bundle > > entries. > > > > I'll add a comment to this effect. > > OK, i take back that early check is not necessary. > > However, count may still be exactly 0 because of overThreshold() (line #151 in > PS3 or #143 in this one). Then, if the the entry is not exporting terminal, > you'd just add it to bundle, but it'd be useless (though not necessarily harmful > either). (See latest message) https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/bundler_impl.go:54: logs := e.GetLogs() On 2015/08/11 18:31:05, tandrii(chromium) wrote: > e.GetLogs() is cheap, and IMO, it's cleaner in this func to just refer to > e.GetLogs() than to logs. I think having the function calls inline in otherwise clean/simple logic is actually ugly. If you don't mind. I'd like to leave this as-is. https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/bundler_impl.go:75: cur = (*bundlerStream)(e) On 2015/08/11 18:31:05, tandrii(chromium) wrote: > i'd merge this and following lines. Oh hey, I'm no longer using "cur" outside of this conditional. I'll do even better than that (Done). https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... File client/internal/logdog/butler/bundler/doc.go (right): https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/doc.go:9: // Butler protobufs together for output. It primarily focused on two protobufs: On 2015/08/11 18:31:05, tandrii(chromium) wrote: > nit: It *is* primarily... Done. https://codereview.chromium.org/1276923003/diff/140001/client/internal/logdog... client/internal/logdog/butler/bundler/doc.go:11: // metadata belongining to a single stream. On 2015/08/11 18:31:05, tandrii(chromium) wrote: > s/belongining/belonging Done.
https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... File client/internal/logdog/butler/bundler/bundler.go (right): https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler.go:41: // Undo reversed the most recent Append. reverses? Can it be called more than once? https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:143: sort.Strings(keys) only for testing, right? https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:164: // If this entry has no logs, it is a terminal entry. Try and export it as hm... there's no more explicit way to denote this? https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:175: // an empty bundle! Discard the full entry (data loss). this needs to be logged somehow. does logdog need its own auxillary stream (e.g. lost+found?). It at least needs to be logged to the local machine. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:190: } shouldn't we stop after the first overage? Or is this so that if a later log is small enough to fit then we could include it too? https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:207: // full entry (data loss). log https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:218: // allocations. In order to do this, we will split the array into two *skeptical* I think this would all be much (much) simpler if you just built a new Logs where you build the omitMap in the first place. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:224: // getBundle in practie will empty "entries". However, even if that weren't s/practie/practice/? https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:256: ec.Logs, e.Logs = e.Logs[:divider], e.Logs[divider:] won't you end up just growing Logs's underlying array forever? I don't think that go can de-allocate part of an array referenced by a slice.
https://chromiumcodereview.appspot.com/1276923003/diff/140001/client/internal... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://chromiumcodereview.appspot.com/1276923003/diff/140001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:54: logs := e.GetLogs() On 2015/08/11 18:31:05, tandrii(chromium) wrote: > e.GetLogs() is cheap, and IMO, it's cleaner in this func to just refer to > e.GetLogs() than to logs. Granted it's cheap, but I don't think it's cleaner. It's a lot of function calls scattered throughout some otherwise-straightforward logic. If you don't mind, I'd like to leave this as-is. https://chromiumcodereview.appspot.com/1276923003/diff/140001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:75: cur = (*bundlerStream)(e) On 2015/08/11 18:31:05, tandrii(chromium) wrote: > i'd merge this and following lines. Oh good catch, I used to care about "cur" outside of this "if" statement, but I don't anymore. I'll make this look a lot better. https://chromiumcodereview.appspot.com/1276923003/diff/140001/client/internal... File client/internal/logdog/butler/bundler/doc.go (right): https://chromiumcodereview.appspot.com/1276923003/diff/140001/client/internal... client/internal/logdog/butler/bundler/doc.go:9: // Butler protobufs together for output. It primarily focused on two protobufs: On 2015/08/11 18:31:05, tandrii(chromium) wrote: > nit: It *is* primarily... Done. https://chromiumcodereview.appspot.com/1276923003/diff/140001/client/internal... client/internal/logdog/butler/bundler/doc.go:11: // metadata belongining to a single stream. On 2015/08/11 18:31:05, tandrii(chromium) wrote: > s/belongining/belonging Done. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... File client/internal/logdog/butler/bundler/bundler.go (right): https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler.go:41: // Undo reversed the most recent Append. On 2015/09/01 03:03:21, iannucci wrote: > reverses? Can it be called more than once? Nope. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... File client/internal/logdog/butler/bundler/bundler_impl.go (right): https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:143: sort.Strings(keys) On 2015/09/01 03:03:21, iannucci wrote: > only for testing, right? I like determinism and predictability :/ So thinking: if keys were unsorted, then successive calls to this could potentially result in different orders of evaluation, which should be fine. May even be nice to mix it up a bit. I'll add a "Deterministic" parameter. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:164: // If this entry has no logs, it is a terminal entry. Try and export it as On 2015/09/01 03:03:21, iannucci wrote: > hm... there's no more explicit way to denote this? Well, kind of. The code drops any entry that doesn't either have logs or be marked terminal. Therefore, at this point, not having logs implies that the entry is marked terminal. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:175: // an empty bundle! Discard the full entry (data loss). On 2015/09/01 03:03:22, iannucci wrote: > this needs to be logged somehow. does logdog need its own auxillary stream (e.g. > lost+found?). It at least needs to be logged to the local machine. Butler will output to STDOUT/STDERR, so Swarming task should pick it up. I'll add a log message. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:190: } On 2015/09/01 03:03:21, iannucci wrote: > shouldn't we stop after the first overage? Or is this so that if a later log is > small enough to fit then we could include it too? Yep that. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:207: // full entry (data loss). On 2015/09/01 03:03:21, iannucci wrote: > log Done. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:218: // allocations. In order to do this, we will split the array into two On 2015/09/01 03:03:21, iannucci wrote: > *skeptical* > > I think this would all be much (much) simpler if you just built a new Logs where > you build the omitMap in the first place. I'm expecting this to get thrashed pretty heavily as logs come in from all over the place. I'd like to avoid unnecessary allocation where the underlying constructs are bounded, which they are in this case. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:224: // getBundle in practie will empty "entries". However, even if that weren't On 2015/09/01 03:03:21, iannucci wrote: > s/practie/practice/? Done. https://chromiumcodereview.appspot.com/1276923003/diff/160001/client/internal... client/internal/logdog/butler/bundler/bundler_impl.go:256: ec.Logs, e.Logs = e.Logs[:divider], e.Logs[divider:] On 2015/09/01 03:03:21, iannucci wrote: > won't you end up just growing Logs's underlying array forever? I don't think > that go can de-allocate part of an array referenced by a slice. Yes, until it empties, in which case it's unreferenced and reclaimed. The GetBundles loop will exhaust all buffered log entries, so this will happen every time.
Description was changed from ========== logdog: Add bundler library. BUG= ========== to ========== logdog: Add bundler library. BUG= ==========
Message was sent while issue was closed.
Closing in favor of Bundler rework: https://codereview.chromium.org/1412063008/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
