Merge pull request #1835 from jaffee/log-nil-fix

pass loggers around properly in gossip

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"])
 		}

gossip/gossip.go

@@ -22,6 +22,7 @@ import (
 	"io/ioutil"
 	"log"
 	"net"
+	"os"
 	"strconv"
 	"strings"
 	"sync"
@@ -50,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
@@ -151,14 +155,20 @@ 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
+		g.stdLogger = logger
 		return nil
 	}
 }
 
+// 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 +176,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 +203,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,8 +213,16 @@ func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*mem
 			return nil, fmt.Errorf("convert port: %s", err)
 		}
 
+		if g.stdLogger == nil {
+			if g.logOutput != nil {
+				g.stdLogger = logger.NewStandardLogger(g.logOutput).Logger()
+			} else {
+				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)
 		}
@@ -233,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{
@@ -318,11 +350,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,

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 {