LogDog: Add collector service implementation.

LogDog: Add collector service implementation.

Add the LogDog Collector daemon. This includes the Collector, its
Dockerfile, and its run script.

Committed: https://github.com/luci/luci-go/commit/02a6f7775b9df1a19bc20237f03fb2a47b583aa3

[dnj (Google), 2016-01-21 04:36:23]

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

[dnj (Google), 2016-01-21 04:36:25]

PTAL! I have spent the last few days unit testing the crap out of this, and I
think it's in a pretty good place. Some parts will drop out when I move to pRPC;
I've marked those in the comments.

https://codereview.chromium.org/1610993002/diff/1/client/cmd/logdog_butler/ma...
File client/cmd/logdog_butler/main.go (left):

https://codereview.chromium.org/1610993002/diff/1/client/cmd/logdog_butler/ma...
client/cmd/logdog_butler/main.go:137: func (a *butlerApplication)
authenticatedContext(ctx context.Context, project string) (context.Context,
error) {
No longer needed w/ Pub/Sub changes.

https://codereview.chromium.org/1610993002/diff/1/client/cmd/logdog_butler/ou...
File client/cmd/logdog_butler/output_pubsub.go (right):

https://codereview.chromium.org/1610993002/diff/1/client/cmd/logdog_butler/ou...
client/cmd/logdog_butler/output_pubsub.go:64: ps := &gcps.Retry{
Use new gcps.Retry and Pub/Sub instantiation.

https://codereview.chromium.org/1610993002/diff/1/client/internal/logdog/butl...
File client/internal/logdog/butler/output/pubsub/pubsubOutput.go (right):

https://codereview.chromium.org/1610993002/diff/1/client/internal/logdog/butl...
client/internal/logdog/butler/output/pubsub/pubsubOutput.go:25: // Publisher is
an interface for something that publishes Pub/Sub messages.
Make a specific interface instead of requiring one that implements all methods
when we only need Publish :)

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/ackbuffe...
File common/gcloud/gcps/ackbuffer/ack.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/ackbuffe...
common/gcloud/gcps/ackbuffer/ack.go:15: type Acknowledger interface {
These changes allow better interoperability between standard interface and the
ackbuffer.

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/ackbuffe...
File common/gcloud/gcps/ackbuffer/ackbuffer.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/ackbuffe...
common/gcloud/gcps/ackbuffer/ackbuffer.go:154: if err := b.cfg.Ack.Ack(b.ctx,
ackIDs...); err != nil {
User must supply a retry-enabled Pub/Sub instance now. This is, fortunately,
super-easy.

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/pubsub.go
File common/gcloud/gcps/pubsub.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/pubsub.g...
common/gcloud/gcps/pubsub.go:32: TopicExists(context.Context, Topic) (bool,
error)
All methods take context.Context now. This is because we may want to cancel them
on a per-call basis (e.g., cancel the Pull, do not cancel the Ack). Previously
the only way to do this was two separate instances! This is more natural.

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/pubsub_i...
File common/gcloud/gcps/pubsub_impl.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/pubsub_i...
common/gcloud/gcps/pubsub_impl.go:21: client  *http.Client
Rearrange to accommodate the end-method Context change.

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/retry.go
File common/gcloud/gcps/retry.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/retry.go...
common/gcloud/gcps/retry.go:13: // Retry wraps a PubSub implementation and
retries on transient errors.
This is super useful. Now we don't have to have one-off retry wrappers every
time someone wants to use Pub/Sub...

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/subscrib...
File common/gcloud/gcps/subscriber/source.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/subscrib...
common/gcloud/gcps/subscriber/source.go:14: type Source interface {
Generalized Source allows for greater interoperability. NewSource below wraps a
Pub/Sub instance, making it easy to get a Source instance.

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/subscrib...
File common/gcloud/gcps/subscriber/subscriber.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/gcloud/gcps/subscrib...
common/gcloud/gcps/subscriber/subscriber.go:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
This changed a fair bit. Originally it dispatched parallel subscription requests
and spat out bundles of messages. It worked like a reactor: you called "Run",
then it blocked until you told it to stop.

Unfortunately, it was only half of the puzzle. You still need to ACK the
messages when you're done with them! This is done by "ackbuffer", but having
that be a separate interface independent of the reactor etc. etc. required some
pretty gross patterns.

Subscriber is now the all-in-one pull-operate-ACK loop. It offers constraints
both on the simultaneous number of pulls that it can do as well as the number of
simultaneous handlers that it will invoke to handle each message.

https://codereview.chromium.org/1610993002/diff/1/common/logdog/butlerproto/p...
File common/logdog/butlerproto/proto.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/logdog/butlerproto/p...
common/logdog/butlerproto/proto.go:129: if r.Metadata.ProtoVersion ==
protocol.Version {
Instead of rejecting messages without a known version, it will just not try and
parse them.

The Collector will reject the messages that have unknown versions.

https://codereview.chromium.org/1610993002/diff/1/common/logdog/butlerproto/p...
common/logdog/butlerproto/proto.go:169: // ProtoVersion is the protocol version
string to use. If empty, the current
Useful for testing: "hey, pretend that you wrote this version".

https://codereview.chromium.org/1610993002/diff/1/common/parallel/parallel.go
File common/parallel/parallel.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/parallel/parallel.go...
common/parallel/parallel.go:19: return Run(nil, gen)
Run is one WorkPool, but more baremetal.

https://codereview.chromium.org/1610993002/diff/1/common/parallel/workPool.go
File common/parallel/workPool.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/parallel/workPool.go...
common/parallel/workPool.go:34: func Run(sem Semaphore, gen func(chan<- func()
error)) error {
Useful b/c the FanOutIn model is great, but sometimes you want to share a
constraint with other FanOutIns.

https://codereview.chromium.org/1610993002/diff/1/common/proto/logdog/service...
File common/proto/logdog/services/gen.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/proto/logdog/service...
common/proto/logdog/services/gen.go:5: //go:generate protoc
--go_out=Mgoogle/protobuf/duration.proto=github.com/luci/luci-go/common/proto/google:.
config.proto storage.proto transport.proto
This is converted to cproto upstream.

https://codereview.chromium.org/1610993002/diff/1/common/retry/retry.go
File common/retry/retry.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/retry/retry.go#newco...
common/retry/retry.go:29: type Generator func() Iterator
This concept is used enough, we might as well codify it.

https://codereview.chromium.org/1610993002/diff/1/common/retry/transient.go
File common/retry/transient.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/retry/transient.go#n...
common/retry/transient.go:41: func TransientOnlyGenerator(gen Generator)
Generator {
Now that we have the concept of a generator, we can chain it!

https://codereview.chromium.org/1610993002/diff/1/common/tsmon/monitor/pubsub.go
File common/tsmon/monitor/pubsub.go (right):

https://codereview.chromium.org/1610993002/diff/1/common/tsmon/monitor/pubsub...
common/tsmon/monitor/pubsub.go:35: return &pubSubMonitor{
Change in response to Pub/Sub changes. "context.Background" is what
cloud.NewContext uses, so this is equivalent.

https://codereview.chromium.org/1610993002/diff/1/server/cmd/logdog_collector...
File server/cmd/logdog_collector/Dockerfile (right):

https://codereview.chromium.org/1610993002/diff/1/server/cmd/logdog_collector...
server/cmd/logdog_collector/Dockerfile:1: # Dockerfile extending the generic Go
image with application files for a
This is used to build the Managed VM. Probably should bump to Go 1.5 :x

https://codereview.chromium.org/1610993002/diff/1/server/cmd/logdog_collector...
File server/cmd/logdog_collector/main.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/cmd/logdog_collector...
server/cmd/logdog_collector/main.go:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
One of the main files to review.

https://codereview.chromium.org/1610993002/diff/1/server/cmd/logdog_collector...
File server/cmd/logdog_collector/run.sh (right):

https://codereview.chromium.org/1610993002/diff/1/server/cmd/logdog_collector...
server/cmd/logdog_collector/run.sh:1: #!/bin/bash
This is invoked at startup by the Dockerfile to pull configuration metadata from
GCE metadata service and pass it as command-line arguments to the Collector
invocation. It's basically environmental glue. The remainder of the config is
then picked up from luci-config.

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coll...
File server/internal/logdog/collector/collector.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coll...
server/internal/logdog/collector/collector.go:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Main file for review.

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coll...
File server/internal/logdog/collector/coordinator.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coll...
server/internal/logdog/collector/coordinator.go:21: type CoordinatorClient
interface {
We can mostly ignore this. I'll probably switch this to use the raw protobuf
interface once I finish the switch to pRPC.

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coll...
File server/internal/logdog/collector/streamstatecache.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coll...
server/internal/logdog/collector/streamstatecache.go:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Another main file for review. Comments should provide some insight as to why it
exists.

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/conf...
File server/internal/logdog/config/config.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/conf...
server/internal/logdog/config/config.go:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Daemon configuration support. Basically owns a luci-config setup. Allows you to
load as a text protobuf from a file for debug purposes.

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coor...
File server/internal/logdog/coordinatorClient/client.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/coor...
server/internal/logdog/coordinatorClient/client.go:5: package coordinatorClient
You can mostly ignore this file. It will be replaced in some or all of its
entirety by pRPC client.

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/serv...
File server/internal/logdog/service/service.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/internal/logdog/serv...
server/internal/logdog/service/service.go:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Generic LogDog daemon support library. This is pulled out from Collector main
because it is super useful for any LogDog daemons that may exist now or in the
future.

https://codereview.chromium.org/1610993002/diff/1/server/logdog/storage/bigta...
File server/logdog/storage/bigtable/bigtable.go (right):

https://codereview.chromium.org/1610993002/diff/1/server/logdog/storage/bigta...
server/logdog/storage/bigtable/bigtable.go:60: "interactive login is required",
Learning as we go.

[dnj, 2016-01-22 01:30:36]

Description was changed from

==========
Add LogDog Collector.

Add the LogDog Collector daemon. This includes the Collector, its
Dockerfile, and its run script.

Additionally, some changes were made to support the Collector:
- Parallel now exposes Run, a more raw method that operates diectly on a
  semaphore. The remainder of its methods have been updated to use Raw.
- Pub/Sub methods now take contexts at method call rather than
  initialization. Several files were updated to reflect this.
- Pub/Sub now supports retry wrappers.
- Retry now formally codifies the idea of a Generator, which is a method
  that produces Iterators.
- Pub/Sub Subscriber is now a complete Pull/ACK engine rather than just
  the subscription piece.
- The BigTable Storage method now includes additional transient error
  strings.

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

to

==========
LogDog: Add collector service implementation.

Add the LogDog Collector daemon. This includes the Collector, its
Dockerfile, and its run script.
==========

[dnj, 2016-01-22 01:37:03]

End of the CL chain, now is nice and split out so this is basically the service
logic implementation.

[dnj, 2016-01-22 01:37:15]

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

[dnj, 2016-01-22 01:37:15]

+martiniss@

[martiniss, 2016-01-23 01:05:41]

Can you put a doc.go in these directories?

server/internal/logdog/collector
server/internal/logdog/config

Will look more on Monday. Thanks for splitting lol, first patch is bigggg.

Looks generally ok, but want to look at the code more.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/collector/coordinator.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/coordinator.go:17: // CoordinatorClient is an
interface to the methods in the Coordiantor service
Typo

[dnj, 2016-01-25 23:14:34]

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

[dnj, 2016-01-25 23:14:34]

+vadimsh@

[martiniss, 2016-01-26 02:24:18]

Still have to read server/internal/logdog/collector/collector.go

Looking good though!

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/collector/streamstatecache.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:47: proto         string
wat this.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/collector/streamstatecache_test.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache_test.go:26: FocusConvey(`Using
a test configuration`, t, func() {
Leftover?

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache_test.go:66: st.ProtoVersion =
""
why are we setting this? Maybe add a comment

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/config/config.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/config.go:31: // it will call os.Exit with
ConfigChangedReturnCode.
Wait os.Exit? Don't see this in the code.

Also I feel like it should just error if KillFunc is nil.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/config/flag.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/flag.go:74: if ccfg.ConfigService == "" {
Why do we check these here? Why not just pass it through and let it fail?

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/config/flag_test.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/flag_test.go:65: So(o.Config,
ShouldHaveSameTypeAs, remote.New(c, ""))
Umm.. What kind of type are we concerned with here? A `config.Interface`, or the
impl version of that. Hard to tell what you're checking from reading this.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/flag_test.go:68: Convey(`Will fail to create
Options if no config service is specified.`, func() {
These tests seem a bit too specific. I mean, I understand why we have them, just
feel like they should be separate? Starting to feel a bit too integratey. But
it's ok for this case, not that much code here.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/coordinatorClient/client.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/coordinatorClient/client.go:147: // If apiBase is not
empty, it will be used to override the
the...............

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/coordinatorClient/client.go:196: // RegisterStream
registers stream metadata with the Coordiantor. The
More typos!

[dnj (Google), 2016-01-26 05:06:05]

Thanks for reviewing! Should be better now.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/collector/coordinator.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/coordinator.go:17: // CoordinatorClient is an
interface to the methods in the Coordiantor service
On 2016/01/23 01:05:41, martiniss wrote:
> Typo

Done.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/collector/streamstatecache.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:47: proto         string
On 2016/01/26 02:24:18, martiniss wrote:
> wat this.

Done.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/collector/streamstatecache_test.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache_test.go:26: FocusConvey(`Using
a test configuration`, t, func() {
On 2016/01/26 02:24:18, martiniss wrote:
> Leftover?

Oops yep. Removed.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache_test.go:66: st.ProtoVersion =
""
On 2016/01/26 02:24:18, martiniss wrote:
> why are we setting this? Maybe add a comment

Added a comment explaining.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/config/config.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/config.go:31: // it will call os.Exit with
ConfigChangedReturnCode.
On 2016/01/26 02:24:18, martiniss wrote:
> Wait os.Exit? Don't see this in the code.
> 
> Also I feel like it should just error if KillFunc is nil.

Ah yeah stale comment. ATM it just ignores things if the function isn't set,
which really is the only safe thing to do

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/config/flag.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/flag.go:74: if ccfg.ConfigService == "" {
On 2016/01/26 02:24:18, martiniss wrote:
> Why do we check these here? Why not just pass it through and let it fail?

I don't really see value to letting the remote library try and ping "http:////"
or w/e. I'm checking it here b/c I have control over it :) Since this is
managing flags, rejecting invalid flags is a reasonable thing to do here.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/config/flag_test.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/flag_test.go:65: So(o.Config,
ShouldHaveSameTypeAs, remote.New(c, ""))
On 2016/01/26 02:24:18, martiniss wrote:
> Umm.. What kind of type are we concerned with here? A `config.Interface`, or
the
> impl version of that. Hard to tell what you're checking from reading this.

remoteImpl, I suppose. It's kind of a silly test, but since remoteImpl is
internal, this is the best way to get a handle to one of them for checking.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/config/flag_test.go:68: Convey(`Will fail to create
Options if no config service is specified.`, func() {
On 2016/01/26 02:24:18, martiniss wrote:
> These tests seem a bit too specific. I mean, I understand why we have them,
just
> feel like they should be separate? Starting to feel a bit too integratey. But
> it's ok for this case, not that much code here.

They aren't really that important, but hurray coverage.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
File server/internal/logdog/coordinatorClient/client.go (right):

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/coordinatorClient/client.go:147: // If apiBase is not
empty, it will be used to override the
On 2016/01/26 02:24:18, martiniss wrote:
> the...............

Done.

https://codereview.chromium.org/1610993002/diff/40001/server/internal/logdog/...
server/internal/logdog/coordinatorClient/client.go:196: // RegisterStream
registers stream metadata with the Coordiantor. The
On 2016/01/26 02:24:18, martiniss wrote:
> More typos!

Done.

[martiniss, 2016-01-27 22:19:44]

https://codereview.chromium.org/1610993002/diff/60001/server/cmd/logdog_colle...
File server/cmd/logdog_collector/Dockerfile (right):

https://codereview.chromium.org/1610993002/diff/60001/server/cmd/logdog_colle...
server/cmd/logdog_collector/Dockerfile:14: # Run the outyet command by default
when the container starts.
outyet?

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/collector/collector.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:38: // StreamStateCacheExpire is
the maximum amount of time that a cahced stream
typo

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:45: // Process ingests an encoded
ButlerLogBundle message. It is goroutine-safe,
What does ingest mean? Maybe I just don't know the definition...

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:72: // Process ingests an encoded
ButlerLogBundle message.
Why not just say it implements the interface?

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:95: // Handle each individual
stream in parallel.
This doesn't seem related to the next line of code.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:131: return nil
wait why don't we return an error if there was one...?

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/collector/collector_test.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:33: type bundleBuilder struct
{
What does this do? Build bundles I guess, but what are bundles? Log entries? is
it a test thing?

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:42: b.base = clock.Now(b)
This confused me at first, but b is a context.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:159: return
fmt.Sprintf("stream %q is registered, but it shoult NOT be.", name)
typo

[iannucci, 2016-01-28 01:15:35]

some comments. martiniss, do you want to do the final review?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
File server/internal/logdog/collector/collector.go (right):

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/collector.go:95: // Handle each individual
stream in parallel.
duplicate comment w/ below?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
File server/internal/logdog/collector/doc.go (right):

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/doc.go:9: // via transport, stashing them in
temporal storage, and registering them with
temporal storage? Like they're put in a metastable quantum chronon loop?

Did you mean temporary?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
File server/internal/logdog/collector/streamstatecache.go (right):

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/streamstatecache.go:23:
defaultStreamStateCacheSize = 1024 * 1024
how was this value determined?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/streamstatecache.go:78: // In addition to
remote cachine via Promise, the state can be updated locally
caching

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/streamstatecache.go:88: terminalIndex int64
probably should also note that this field must not be directly read/manipulated,
but instead manipulated via the helper methods.

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/streamstatecache.go:90: p        
promise.Promise
inner type?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/collector/streamstatecache.go:107: // We need to lock
around our terminalIndex.
erm... where's the lock? It looks like you read it without locking?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
File server/internal/logdog/config/config.go (right):

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/config/config.go:39: func (o *Options) startKillPoller(c
context.Context, hash string) {
isn't this actually a config poller that happens to have the behavior that it
kills the program (in production) when the config changes?

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
File server/internal/logdog/config/filesystem.go (right):

https://chromiumcodereview.appspot.com/1610993002/diff/60001/server/internal/...
server/internal/logdog/config/filesystem.go:26: type fileConfig struct {
this doesn't match the source file name?

[dnj, 2016-01-29 20:46:52]

PTAL. I've updated this for pRPC and cleaned some things up a bit. It's _mostly_
the same stuff, but my apologizes for the shake-up. It was getting really
difficult to maintain two versions of this when working on pRPC elsewhere.

https://codereview.chromium.org/1610993002/diff/60001/server/cmd/logdog_colle...
File server/cmd/logdog_collector/Dockerfile (right):

https://codereview.chromium.org/1610993002/diff/60001/server/cmd/logdog_colle...
server/cmd/logdog_collector/Dockerfile:14: # Run the outyet command by default
when the container starts.
On 2016/01/27 22:19:43, martiniss wrote:
> outyet?

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/collector/collector.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:38: // StreamStateCacheExpire is
the maximum amount of time that a cahced stream
On 2016/01/27 22:19:43, martiniss wrote:
> typo

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:45: // Process ingests an encoded
ButlerLogBundle message. It is goroutine-safe,
On 2016/01/27 22:19:43, martiniss wrote:
> What does ingest mean? Maybe I just don't know the definition...

Ingest as in like, eating something.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:72: // Process ingests an encoded
ButlerLogBundle message.
On 2016/01/27 22:19:43, martiniss wrote:
> Why not just say it implements the interface?

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:95: // Handle each individual
stream in parallel.
On 2016/01/28 01:15:35, iannucci wrote:
> duplicate comment w/ below?

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector.go:131: return nil
On 2016/01/27 22:19:43, martiniss wrote:
> wait why don't we return an error if there was one...?

I actually mixed transient and non-transient code paths up here in the refactor.
I fixed.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/collector/collector_test.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:33: type bundleBuilder struct
{
On 2016/01/27 22:19:44, martiniss wrote:
> What does this do? Build bundles I guess, but what are bundles? Log entries?
is
> it a test thing?

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:42: b.base = clock.Now(b)
On 2016/01/27 22:19:44, martiniss wrote:
> This confused me at first, but b is a context.

Acknowledged.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:159: return
fmt.Sprintf("stream %q is registered, but it shoult NOT be.", name)
On 2016/01/27 22:19:43, martiniss wrote:
> typo

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/collector/doc.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/doc.go:9: // via transport, stashing them in
temporal storage, and registering them with
On 2016/01/28 01:15:35, iannucci wrote:
> temporal storage? Like they're put in a metastable quantum chronon loop?
> 
> Did you mean temporary?

Changed to intermediate.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/collector/streamstatecache.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:23:
defaultStreamStateCacheSize = 1024 * 1024
On 2016/01/28 01:15:35, iannucci wrote:
> how was this value determined?

Big number. I'll make this configurable.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:78: // In addition to
remote cachine via Promise, the state can be updated locally
On 2016/01/28 01:15:35, iannucci wrote:
> caching

Done.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:88: terminalIndex int64
On 2016/01/28 01:15:35, iannucci wrote:
> probably should also note that this field must not be directly
read/manipulated,
> but instead manipulated via the helper methods.

I feel like that's the default for internal members.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:90: p        
promise.Promise
On 2016/01/28 01:15:35, iannucci wrote:
> inner type?

Will document.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/collector/streamstatecache.go:107: // We need to lock
around our terminalIndex.
On 2016/01/28 01:15:35, iannucci wrote:
> erm... where's the lock? It looks like you read it without locking?

Oh whoops, old comment before atomic goodness.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/config/config.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/config/config.go:39: func (o *Options) startKillPoller(c
context.Context, hash string) {
On 2016/01/28 01:15:35, iannucci wrote:
> isn't this actually a config poller that happens to have the behavior that it
> kills the program (in production) when the config changes?

Yes? I named it this b/c it's polling to kill, but I can see where there could
be some confusion.

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
File server/internal/logdog/config/filesystem.go (right):

https://codereview.chromium.org/1610993002/diff/60001/server/internal/logdog/...
server/internal/logdog/config/filesystem.go:26: type fileConfig struct {
On 2016/01/28 01:15:35, iannucci wrote:
> this doesn't match the source file name?

Done.

[dnj (Google), 2016-02-02 22:06:24]

martiniss@, iannucci@, can one of you do a final pass on the new CL?

[iannucci, 2016-02-05 23:41:01]

lgtm

https://codereview.chromium.org/1610993002/diff/80001/common/proto/logdog/svc...
File common/proto/logdog/svcconfig/config.proto (right):

https://codereview.chromium.org/1610993002/diff/80001/common/proto/logdog/svc...
common/proto/logdog/svcconfig/config.proto:49: // The maximum amount of time
that cached stream state is valid. If <= 0, a
nit: newline

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/collector/collector_test.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:45: for _, v := range []bool{
for _, phrase := range []string{"disabled", "enabled"} {
  v := phrase == "enabled"
  ...
}
?

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/collector/coordinator/cache.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/cache.go:45: // NewCache creates a
new Cache instance that wraps a Coordinator instance.
wraps a Coordinator with a cache?

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/cache.go:61: // getOrRegister is a
goroutine-safe blocking call that synchronizes log stream
comment desync

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/collector/coordinator/coordinator.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/coordinator.go:18: // Coordinator
is a Collector-driven interface to a Coordinator service.
collector-driven? idkwym :(

CoordinatorClient?

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/coordinator.go:37: Purged       
bool               // Is the stream purged?
why not go-style comments (above the field)?

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/config/fileConfig.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/config/fileConfig.go:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
I feel like this sort of thing should really be somewhere in common/config. It's
not related to logdog at all, is it?

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/config/flag.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/config/flag.go:50: ccfg, err := client.GetConfig(c,
&services.Void{})
Empty message?

actually you could probably pass nil here, right?

[dnj (Google), 2016-02-06 04:10:37]

Awesome, thanks!

https://codereview.chromium.org/1610993002/diff/80001/common/proto/logdog/svc...
File common/proto/logdog/svcconfig/config.proto (right):

https://codereview.chromium.org/1610993002/diff/80001/common/proto/logdog/svc...
common/proto/logdog/svcconfig/config.proto:49: // The maximum amount of time
that cached stream state is valid. If <= 0, a
On 2016/02/05 23:41:01, iannucci wrote:
> nit: newline

Done.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/collector/collector_test.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/collector_test.go:45: for _, v := range []bool{
On 2016/02/05 23:41:01, iannucci wrote:
> for _, phrase := range []string{"disabled", "enabled"} {
>   v := phrase == "enabled"
>   ...
> }
> ?

Done.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/collector/coordinator/cache.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/cache.go:45: // NewCache creates a
new Cache instance that wraps a Coordinator instance.
On 2016/02/05 23:41:01, iannucci wrote:
> wraps a Coordinator with a cache?

Done.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/cache.go:61: // getOrRegister is a
goroutine-safe blocking call that synchronizes log stream
On 2016/02/05 23:41:01, iannucci wrote:
> comment desync

Done.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/collector/coordinator/coordinator.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/coordinator.go:18: // Coordinator
is a Collector-driven interface to a Coordinator service.
On 2016/02/05 23:41:01, iannucci wrote:
> collector-driven? idkwym :(
> 
> CoordinatorClient?

Done.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/collector/coordinator/coordinator.go:37: Purged       
bool               // Is the stream purged?
On 2016/02/05 23:41:01, iannucci wrote:
> why not go-style comments (above the field)?

These are basically projections of the actual log stream state. All comments
would otherwise go, "Path is the path field, ProtoVerison is the protobuf
version field." I this is more concise and effective for this sort of struct.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/config/fileConfig.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/config/fileConfig.go:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/02/05 23:41:01, iannucci wrote:
> I feel like this sort of thing should really be somewhere in common/config.
It's
> not related to logdog at all, is it?

If it were fully implemented I'd agree, but ATM it's a very specific partial
implementation based on what this package uses.

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
File server/internal/logdog/config/flag.go (right):

https://codereview.chromium.org/1610993002/diff/80001/server/internal/logdog/...
server/internal/logdog/config/flag.go:50: ccfg, err := client.GetConfig(c,
&services.Void{})
On 2016/02/05 23:41:01, iannucci wrote:
> Empty message?
> 
> actually you could probably pass nil here, right?

That's not how g/pRPC works, unfortunately. It expects an actual message value
in the RPC call.

I finished an implementation of client/server nil passing, but then looked at
gRPC code and noted that it didn't do this, so I retracted.

[dnj (Google), 2016-02-06 04:12:07]

The CQ bit was checked by dnj@google.com

[commit-bot: I haz the power, 2016-02-06 04:12:15]

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

[commit-bot: I haz the power, 2016-02-06 04:13:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-06 04:13:06]

Try jobs failed on following builders:
  Luci-go Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Preci...)
  Luci-go Mac Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Mac%20Tester/...)
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[dnj (Google), 2016-02-06 04:14:02]

The CQ bit was checked by dnj@google.com

[dnj (Google), 2016-02-06 04:14:03]

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/1610993002/#ps100001
(title: "Comments, rebase.")

[commit-bot: I haz the power, 2016-02-06 04:14:05]

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

[commit-bot: I haz the power, 2016-02-06 04:16:34]

Description was changed from

==========
LogDog: Add collector service implementation.

Add the LogDog Collector daemon. This includes the Collector, its
Dockerfile, and its run script.
==========

to

==========
LogDog: Add collector service implementation.

Add the LogDog Collector daemon. This includes the Collector, its
Dockerfile, and its run script.

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

[commit-bot: I haz the power, 2016-02-06 04:16:35]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/02a6f7775b9df1a19bc20237f03fb2a47b583aa3