From 6f4dee5e31f30547d18b1906589d327a08e813f7 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 18 Jan 2019 14:06:10 -0600 Subject: [PATCH 1/3] pass loggers around properly in gossip --- gossip/gossip.go | 39 ++++++++++++++++++++++++++++++++++----- server/server.go | 1 + 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/gossip/gossip.go b/gossip/gossip.go index 5e83c7ddf..d5aaebc59 100644 --- a/gossip/gossip.go +++ b/gossip/gossip.go @@ -22,6 +22,7 @@ import ( "io/ioutil" "log" "net" + "os" "strconv" "strings" "sync" @@ -151,7 +152,11 @@ func WithTransport(transport *Transport) memberSetOption { } } -// WithLogger is a functional option for providing a logger to NewMemberSet. +// WithLogger is a functional option for providing a Go logger to NewMemberSet. +// If the memberSet's transport is nil, this logger will be used when creating +// one. If WithLogOutput is not used, this logger will be passed to memberlist +// for it to use internally. This logger is not used for logging by code in this +// (gossip) package - for that, use the WithPilosaLogger option. func WithLogger(logger *log.Logger) memberSetOption { return func(g *memberSet) error { g.logger = logger @@ -159,6 +164,8 @@ func WithLogger(logger *log.Logger) memberSetOption { } } +// WithLogOutput allows one to pass a Writer which will in turn be passed to +// memberlist for use in logging. func WithLogOutput(o io.Writer) memberSetOption { return func(g *memberSet) error { g.logOutput = o @@ -166,7 +173,20 @@ func WithLogOutput(o io.Writer) memberSetOption { } } -// NewMemberSet returns a new instance of GossipMemberSet based on options. +// WithPilosaLogger allows one to configure a memberSet with a logger of their +// choice which satisfies the pilosa logger interface. +func WithPilosaLogger(l logger.Logger) memberSetOption { + return func(g *memberSet) error { + g.Logger = l + return nil + } +} + +// NewMemberSet returns a new instance of GossipMemberSet based on options. The +// logging options which can be passed to NewMemberSet are complicated for +// historical reasons - please pass WithPilosaLogger, and either WithLogOutput +// or WithLogger. If you pass WithLogOutput, be sure to also pass in a Transport +// using WithTransport. func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*memberSet, error) { host := api.Node().URI.Host g := &memberSet{ @@ -180,7 +200,8 @@ func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*mem return nil, errors.Wrap(err, "executing option") } } - ger := newEventReceiver(g.logger, api) + + ger := newEventReceiver(g.Logger, api) g.eventReceiver = ger if g.transport == nil { @@ -189,6 +210,14 @@ func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*mem return nil, fmt.Errorf("convert port: %s", err) } + if g.logger == nil { + if g.logOutput != nil { + g.logger = logger.NewStandardLogger(g.logOutput).Logger() + } else { + g.logger = log.New(os.Stderr, "", log.LstdFlags) + } + } + // Set up the transport. transport, err := NewTransport(host, port, g.logger) if err != nil { @@ -318,11 +347,11 @@ type eventReceiver struct { ch chan memberlist.NodeEvent papi *pilosa.API - logger *log.Logger + logger logger.Logger } // newEventReceiver returns a new instance of GossipEventReceiver. -func newEventReceiver(logger *log.Logger, papi *pilosa.API) *eventReceiver { +func newEventReceiver(logger logger.Logger, papi *pilosa.API) *eventReceiver { ger := &eventReceiver{ ch: make(chan memberlist.NodeEvent, 1), logger: logger, diff --git a/server/server.go b/server/server.go index c80f8268e..5f99e510e 100644 --- a/server/server.go +++ b/server/server.go @@ -320,6 +320,7 @@ func (m *Command) setupNetworking() error { m.Config.Gossip, m.API, gossip.WithLogOutput(&filteredWriter{logOutput: m.logOutput, v: m.Config.Verbose}), + gossip.WithPilosaLogger(m.logger), gossip.WithTransport(m.gossipTransport), ) if err != nil { From 44270fe9fd90ab95555965a3f5d0f3bc39c9b826 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 21 Jan 2019 12:11:05 -0600 Subject: [PATCH 2/3] rename memberlist.logger and add explanatory comments --- gossip/gossip.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/gossip/gossip.go b/gossip/gossip.go index d5aaebc59..e3c552f88 100644 --- a/gossip/gossip.go +++ b/gossip/gossip.go @@ -51,8 +51,11 @@ type memberSet struct { Logger logger.Logger - logger *log.Logger + // stdLogger is only used when passed into memberlist library things that take a std library logger rather than an interface. + stdLogger *log.Logger + // logOutput is similar to stdLogger in that it's passed to memberlist things which can't take a pilosa Logger. logOutput io.Writer + transport *Transport eventReceiver *eventReceiver @@ -159,7 +162,7 @@ func WithTransport(transport *Transport) memberSetOption { // (gossip) package - for that, use the WithPilosaLogger option. func WithLogger(logger *log.Logger) memberSetOption { return func(g *memberSet) error { - g.logger = logger + g.stdLogger = logger return nil } } @@ -210,16 +213,16 @@ func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*mem return nil, fmt.Errorf("convert port: %s", err) } - if g.logger == nil { + if g.stdLogger == nil { if g.logOutput != nil { - g.logger = logger.NewStandardLogger(g.logOutput).Logger() + g.stdLogger = logger.NewStandardLogger(g.logOutput).Logger() } else { - g.logger = log.New(os.Stderr, "", log.LstdFlags) + g.stdLogger = log.New(os.Stderr, "", log.LstdFlags) } } // Set up the transport. - transport, err := NewTransport(host, port, g.logger) + transport, err := NewTransport(host, port, g.stdLogger) if err != nil { return nil, fmt.Errorf("new tranport: %s", err) } @@ -262,7 +265,7 @@ func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*mem if g.logOutput != nil { conf.LogOutput = g.logOutput } else { - conf.Logger = g.logger + conf.Logger = g.stdLogger } g.config = &config{ From bb9f3a95d263049983d4b756db8bddb6ae08e512 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 21 Jan 2019 15:17:33 -0600 Subject: [PATCH 3/3] move legacy field check to non-concurrent code --- executor.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/executor.go b/executor.go index c35d0276a..b17cf95e2 100644 --- a/executor.go +++ b/executor.go @@ -914,6 +914,12 @@ func (e *executor) executeGroupBy(ctx context.Context, index string, c *pql.Call // TODO support TopN in here would be really cool - and pretty easy I think. childRows := make([]RowIDs, len(c.Children)) for i, child := range c.Children { + // Check "field" first for backwards compatibility, then set _field. + // TODO: remove at Pilosa 2.0 + if fieldName, ok := child.Args["field"].(string); ok { + child.Args["_field"] = fieldName + } + if child.Name != "Rows" { return nil, errors.Errorf("'%s' is not a valid child query for GroupBy, must be 'Rows'", child.Name) } @@ -2767,11 +2773,6 @@ func newGroupByIterator(rowIDs []RowIDs, children []*pql.Call, filter *Row, inde var ok bool ignorePrev := false for i, call := range children { - // Check "field" first for backwards compatibility. - // TODO: remove at Pilosa 2.0 - if fieldName, ok = call.Args["field"].(string); ok { - call.Args["_field"] = fieldName - } if fieldName, ok = call.Args["_field"].(string); !ok { return nil, errors.Errorf("%s call must have field with valid (string) field name. Got %v of type %[2]T", call.Name, call.Args["_field"]) }