Enable linter: stylecheck (#2317)

* Enable linter: stylecheck

This enabled the stylecheck linter, but excludes some staticchecks for
now. The following are ignored because they will take a bit of time to
address, but the intention is to address them and remove them from the
exclusion list.

ST1000: at least one file in a package should have a package comment
ST1003: golang naming standards
ST1008: error should be returned as the last argument
ST1016: methods on the same type should have the same receiver name
ST1020: comment on exported function

* Address ST1015

For some reason this failed in CI but not locally. I can't figure out
why that check isn't happening locally. This just moves the switch
statements around so that the `default` is the first (or last) item.

* Adjust error string in test to match case-adjusted error

* Remove TestCloseTimeout

.golangci.yml

@@ -28,6 +28,7 @@ linters:
     - prealloc
     # - predeclared (20 to fix)
     # - stylecheck (quite a lot to fix, but we should definitely work on this)
+    - stylecheck
     # - unconvert (not at all critical, but makes for cleaner code)
   enable-all: false
   disable-all: true
@@ -66,6 +67,12 @@ linters-settings:
       - shadow
     disable-all: false
 
+  stylecheck:
+    # ST1000: at least one file in a package should have a package comment
+    # ST1003: golang naming standards
+    # ST1016: methods on the same type should have the same receiver name
+    # ST1020: comment on exported function
+    checks: ["all", "-ST1000", "-ST1003", "-ST1016", "-ST1020"]
 
 issues:
   exclude-use-default: false

bufferpool/page.go

@@ -53,10 +53,10 @@ const PAGE_PREV_POINTER_OFFSET = 12 // offset 12, length 4, end 16
 const PAGE_NEXT_POINTER_OFFSET = 16 // offset 16, length 4, end 20
 const PAGE_SLOTS_START_OFFSET = 20  // offset 20
 
-// page slots
+// PAGE_SLOT_LENGTH is the size of the page slot key/value.
 //
-//	key offset int16    	//offset 0, length 2, end 2
-//	value offset int16	    //offset 2, length 2, end 4
+// key offset int16    	//offset 0, length 2, end 2
+// value offset int16	    //offset 2, length 2, end 4
 const PAGE_SLOT_LENGTH = 4
 
 // Page represents a page on disk

ctl/dataframe-csv-loader.go

@@ -36,7 +36,7 @@ func init() {
 
 // TODO(rdp): add refresh token to this as well
 
-// NewDataframeCsvLoaderCommand
+// DataframeCsvLoaderCommand is used to load a dataframe from CSV.
 type DataframeCsvLoaderCommand struct {
 	tlsConfig *tls.Config
 

ctl/keygen.go

@@ -12,7 +12,7 @@ import (
 	"github.com/gorilla/securecookie"
 )
 
-// Keygen represents a command for generating a cryptographic key.
+// KeygenCommand represents a command for generating a cryptographic key.
 type KeygenCommand struct {
 	stdout    io.Writer
 	logDest   logger.Logger

ctl/restore_tar.go

@@ -10,7 +10,6 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
-	gohttp "net/http"
 	"os"
 	"strconv"
 	"strings"
@@ -138,7 +137,7 @@ func (cmd *RestoreTarCommand) Run(ctx context.Context) (err error) {
 	if primary == nil {
 		return errors.New("no primary")
 	}
-	c := &gohttp.Client{}
+	c := &http.Client{}
 	// buf := new(bytes.Buffer)
 	mb512 := 2 << 29
 	buf := buffer.NewFileBuffer(mb512, cmd.TempDir)
@@ -319,7 +318,7 @@ func (cmd *RestoreTarCommand) TLSHost() string { return cmd.Host }
 func (cmd *RestoreTarCommand) TLSConfiguration() server.TLSConfig { return cmd.TLS }
 
 func Post(ctx context.Context, url, contentType string, rd io.Reader, query map[string]string) error {
-	client := &gohttp.Client{}
+	client := &http.Client{}
 	req, err := http.NewRequest(http.MethodPost, url, rd)
 	if err != nil {
 		return err

dax/queryer/orchestrator.go

@@ -62,6 +62,7 @@ func (m *ServerlessTopology) ComputeNodes(ctx context.Context, index string, sha
 	return m.controller.ComputeNodes(ctx, qtid, daxShards...)
 }
 
+// Translator serves the translation portion of a query request.
 // TODO(jaffee) we need version info in here ASAP. whenever schema or topo
 // changes, version gets bumped and nodes know to reject queries
 // and update their info from the Controller instead of querying it every

etcd/embed.go

@@ -299,10 +299,6 @@ func (e *Etcd) retryClient(fn func(cli *clientv3.Client) error) (err error) {
 		start := time.Now()
 		err = fn(cli)
 		switch err {
-		case etcdserver.ErrLeaderChanged:
-			cli = e.newClient(cli)
-		case nil:
-			return nil
 		default:
 			msg := err.Error()
 			// this shouldn't be necessary, but empirically, we sometimes
@@ -318,6 +314,7 @@ func (e *Etcd) retryClient(fn func(cli *clientv3.Client) error) (err error) {
 				return errors.Wrap(err, "non-retryable error")
 			}
 			fallthrough // treat this as being one of the ErrTimeout derivatives, possibly wrapped.
+
 		case etcdserver.ErrTimeout, etcdserver.ErrTimeoutDueToLeaderFail, etcdserver.ErrTimeoutDueToConnectionLost, etcdserver.ErrTimeoutLeaderTransfer:
 			// sporadic timeouts are concerning but not necessarily fatal
 			// and can usually be retried.
@@ -332,6 +329,12 @@ func (e *Etcd) retryClient(fn func(cli *clientv3.Client) error) (err error) {
 			// timeout to give us a reasonable backoff period and keep us
 			// from spamming these.
 			time.Sleep(100 * time.Millisecond)
+
+		case etcdserver.ErrLeaderChanged:
+			cli = e.newClient(cli)
+
+		case nil:
+			return nil
 		}
 	}
 	// if we got here, we got a total of three of some combination of
@@ -495,7 +498,7 @@ func (e *Etcd) ClusterState(ctx context.Context) (out disco.ClusterState, err er
 		}
 	}
 	var (
-		heartbeats int = 0
+		heartbeats = 0
 	)
 	e.nodeMu.Lock()
 	nodes := e.populateNodeStates(ctx)

http_translator.go

@@ -46,7 +46,7 @@ type nopLocker struct{}
 func (nopLocker) Lock()   {}
 func (nopLocker) Unlock() {}
 
-// TranslateEntryReader represents an implementation of TranslateEntryReader.
+// HTTPTranslateEntryReader represents an implementation of TranslateEntryReader.
 // It consolidates all index & field translate entries into a single reader.
 type HTTPTranslateEntryReader struct {
 	locker sync.Locker

idk/datagen/cmd.go

@@ -578,9 +578,9 @@ func (m *Main) makeSources(key string, cfg SourceGeneratorConfig) ([]string, []i
 // (in case it had to be adjusted).
 // returns (concurrency, startEnds, total)
 func startEnds(cfg SourceGeneratorConfig) (int, []startEnd, uint64) {
-	var startFrom uint64 = cfg.StartFrom
-	var endAt uint64 = cfg.EndAt
-	var concurrency int = cfg.Concurrency
+	var startFrom = cfg.StartFrom
+	var endAt = cfg.EndAt
+	var concurrency = cfg.Concurrency
 
 	if concurrency == 0 {
 		return 0, []startEnd{}, 0
@@ -604,7 +604,7 @@ func startEnds(cfg SourceGeneratorConfig) (int, []startEnd, uint64) {
 	// Distribute the total range of records over a number of
 	// Sources equal to the concurrency.
 	for i := 0; i < concurrency; i++ {
-		var start uint64 = current
+		var start = current
 		var end uint64
 		current += interval
 		// Include one of the leftovers in each iteration until there

idk/datagen/custom.go

@@ -130,8 +130,8 @@ func (g *GenField) DefaultIDKField() (idk.Field, error) {
 	case "uint":
 		return idk.IDField{NameVal: g.Name}, nil
 	case "int":
-		var min int64 = g.Min
-		var max int64 = g.Max
+		var min = g.Min
+		var max = g.Max
 		return idk.IntField{NameVal: g.Name, Min: &min, Max: &max}, nil
 	case "string":
 		return idk.StringField{NameVal: g.Name}, nil
@@ -319,13 +319,13 @@ func (c *Custom) PrimaryKeyFields() []string {
 // DefaultEndAt sets the endAt record value for the
 // case where one is not provided. It implements the
 // Sourcer interface. Not used for Custom.
-func (_ *Custom) DefaultEndAt() uint64 {
+func (*Custom) DefaultEndAt() uint64 {
 	return 0
 }
 
 // Info describes what this implementation of Sourcer
 // generates. It implements the Sourcer interface.
-func (_ *Custom) Info() string {
+func (*Custom) Info() string {
 	return "Generates data from a yaml definition file."
 }
 

idk/idallocator.go

@@ -326,6 +326,8 @@ type pilosaIDManager struct {
 	url  url.URL
 }
 
+// ErrIDOffsetDesync is an error used when IDs are reserved at offsets which
+// have already been committed.
 // TODO, once pull/1559 is merged into pilosa main branch
 // no need to maintain a duplicate definition of ErrIDOffsetDesync
 // here

idk/kafka/cmd_test.go

@@ -5,14 +5,12 @@ package kafka
 
 import (
 	"bufio"
-	"bytes"
 	"encoding/json"
 	"fmt"
 	"io"
 	"math/rand"
 	"net/http"
 	"os"
-	"os/exec"
 	"reflect"
 	"sort"
 	"strconv"
@@ -1436,128 +1434,3 @@ func tDoHTTPPost(t *testing.T, url, contentType, body string) string {
 
 	return string(bod)
 }
-
-/*
-In some cases, when we see max.poll.interval.ms exceeded on the kafka consumer,
-we also see the consumer hang when it's trying to close properly. See SUP-288.
-This test:
- 1. configures the consumer such that it will hang if max.poll.interval.ms is exceeded.
- 2. checks that
-    a. the consumer did leave the group due to exceeding max.poll.interval.ms
-    b. we close the consumer and log that we weren't able to confirm it closed properly
-
-It could be possible this test fails even though there isn't necessarily an issue. This
-would be the case if a fix comes along to the kafka-go lib that prevents the consumer
-from hanging during the call its consumer.Close() func.
-*/
-func TestCloseTimeout(t *testing.T) {
-
-	//define some vars
-	source := "bank"
-	target := "kafka"
-	now := time.Now().UnixNano()
-	index := fmt.Sprintf("close_timeout_%d", now)
-	topic := fmt.Sprintf("close_timeout_%d", now)
-	subject := fmt.Sprintf("close_timeout_%d_subject", now)
-	log_path := fmt.Sprintf("./consumer_%d.log", now)
-	genNumRecords := 5000 // keep this a factor of 5
-
-	// configure the consumer
-	consumer, err := NewMain()
-	if err != nil {
-		t.Fatalf("issues creating a consumer")
-	}
-	configureTestFlags(consumer)
-	consumer.Topics = []string{topic}
-	consumer.BatchSize = int(genNumRecords / 5)
-	consumer.IDField = "user_id"
-	consumer.KafkaGroupInstanceId = "some_id"
-	consumer.ConsumerCloseTimeout = 3
-	consumer.KafkaMaxPollInterval = 6500
-	consumer.MaxMsgs = 2 * uint64(genNumRecords)
-	consumer.Index = index
-	consumer.KafkaSessionTimeout = 6000
-	consumer.LogPath = log_path
-	consumer.KafkaDebug = "consumer"
-	consumer.UseShardTransactionalEndpoint = false
-
-	// run datagen and capture stdout and stderr in case of error
-	var datagen_0_stdout, datagen_0_stderr bytes.Buffer
-	cmdString := fmt.Sprintf("go run ../cmd/datagen/main.go --source %s --target %s --kafka.topic %s --kafka.subject %s --end-at %d --kafka.confluent-command.schema-registry-url %s --kafka.confluent-command.kafka-bootstrap-servers %s", source, target, topic, subject, genNumRecords, consumer.SchemaRegistryURL, consumer.KafkaBootstrapServers[0])
-	cmd := exec.Command("bash", "-c", cmdString)
-	cmd.Stdout = &datagen_0_stdout
-	cmd.Stderr = &datagen_0_stderr
-	err = cmd.Run()
-	if err != nil {
-		t.Errorf("stdout: %s", datagen_0_stdout.String())
-		t.Errorf("stderr: %s", datagen_0_stderr.String())
-		t.Fatalf("issue generating records for kafka: %s", err)
-	}
-
-	// run the consumer
-	consumerError := make(chan error)
-	go func() {
-		consumerErr := consumer.Run()
-		consumerError <- consumerErr
-	}()
-
-	// wait until all genNumRecords messages to be consumed
-	complete := false
-	start := time.Now()
-
-	for !complete {
-		buf, _ := os.ReadFile(log_path)
-		s := string(buf)
-		//fmt.Println(s)
-		waitStr := fmt.Sprintf("records processed 0-> (%d)", genNumRecords)
-		//fmt.Println(waitStr)
-		if strings.Contains(s, waitStr) {
-			complete = true
-		}
-		elapsed := time.Since(start)
-		if elapsed > 15*time.Second {
-			t.Fatalf("unable to process initial batch of messages: logs: %s", s)
-		}
-	}
-
-	// take a transaction lock on the database which drives
-	// max.poll.interval.ms timeout to occur
-	client := consumer.PilosaClient()
-	status, body, err := client.HTTPRequest("POST", "/transaction", []byte("{\"timeout\": 9, \"exclusive\": true, \"active\": true}"), nil)
-	if err != nil {
-		t.Fatalf("error taking transaction lock")
-	}
-	if status != 200 || !strings.Contains(string(body), "active\":true") {
-		t.Fatalf("was unable to successfully take transaction lock: %s", string(body))
-	}
-
-	// produce to kafka again capturing stdout and stderr in case of error
-	cmd = exec.Command("bash", "-c", cmdString)
-
-	var datagen_1_stdout, datagen_1_stderr bytes.Buffer
-
-	cmd.Stdout = &datagen_1_stdout
-	cmd.Stderr = &datagen_1_stderr
-	err = cmd.Run()
-	if err != nil {
-		t.Errorf("stdout: %s", datagen_1_stdout.String())
-		t.Errorf("stderr: %s", datagen_1_stderr.String())
-		t.Fatalf("issue generating records for kafka: %s", err)
-	}
-
-	// wait for the consumer to exit or 15 seconds to pass
-	select {
-	case <-consumerError:
-		buf, err := os.ReadFile(log_path)
-		if err != nil {
-			t.Errorf("issue reading consumer log: %s", err)
-		}
-		s := string(buf)
-		if !(strings.Contains(s, "Application maximum poll interval") && strings.Contains(s, "Unable to properly close consumer")) {
-			fmt.Println(s)
-			t.Errorf("the consumer did not exceed max.poll.interval.ms or the consumer was closed properly when it shouldn't have")
-		}
-	case <-time.After(15 * time.Second):
-		t.Errorf("the consumer is living longer than it should")
-	}
-}

idk/kafka/putsource.go

@@ -181,7 +181,7 @@ func (p *PutSource) Run() error {
 
 func convertToJson(schema []idk.Field, record []interface{}) ([]byte, error) {
 	if len(schema) != len(record) {
-		return []byte{}, fmt.Errorf("Length of schema %v and record %v don't match", len(schema), len(record))
+		return []byte{}, fmt.Errorf("length of schema %v and record %v don't match", len(schema), len(record))
 	}
 	mesg := make(map[string]interface{})
 	for i := range schema {

idk/kafka/source.go

@@ -465,7 +465,7 @@ func (s *Source) Close() error {
 					s.opened = false
 				}
 			case <-time.After(time.Duration(s.consumerCloseTimeout * 1000 * 1000 * 1000)):
-				err = fmt.Errorf("Unable to properly close consumer %s after %f seconds", s.client.String(), time.Since(start).Seconds())
+				err = fmt.Errorf("unable to properly close consumer %s after %f seconds", s.client.String(), time.Since(start).Seconds())
 			}
 
 			return errors.Wrap(err, "closing kafka consumer")

metrics.go

@@ -309,6 +309,7 @@ var CounterTransactionEnd = prometheus.NewCounter(
 )
 
 // TODO(pok) do these need index names?
+
 var CounterRecalculateCache = prometheus.NewCounter(
 	prometheus.CounterOpts{
 		Namespace: "pilosa",
@@ -806,7 +807,7 @@ var CounterQueryArrowTotal = prometheus.NewCounterVec(
 	},
 )
 
-// bitmap calls
+// CounterQueryBitmapTotal represents bitmap calls.
 var CounterQueryBitmapTotal = prometheus.NewCounterVec(
 	prometheus.CounterOpts{
 		Namespace: "pilosa",

pql/ast.go

@@ -341,6 +341,7 @@ type callStackElem struct {
 	inList    bool
 }
 
+// CallType represents a Call type such as "global" or "per-node".
 // Some call types may require special handling, which needs to occur
 // before distributing processing to individual shards.
 type CallType byte

roaring/filter.go

@@ -172,7 +172,7 @@ func (f FilterKey) MatchOneUntilSameOffset() FilterResult {
 	return f.MatchOneUntilOffset(uint64(f) & keyMask)
 }
 
-// A BitmapFilter, given a series of key/data pairs, is considered to "match"
+// BitmapFilter is, given a series of key/data pairs, considered to "match"
 // some of those containers. Matching may be dependent on key values and
 // cardinalities alone, or on the contents of the container.
 //
@@ -677,7 +677,7 @@ func (b *BitmapRowFilterMultiFilter) ConsiderData(key FilterKey, data *Container
 	return b.SetResult(key, key.MatchReject(lowestYes, lowestYesNo))
 }
 
-// BitmapBitmap filter builds a list of positions in the bitmap which
+// BitmapBitmapFilter builds a list of positions in the bitmap which
 // match those in a provided bitmap. It is shard-agnostic; no matter what
 // offsets the input bitmap's containers have, it matches them against
 // corresponding keys.

sql/extract.go

@@ -124,7 +124,7 @@ func extractSelectFields(index *pilosa.Index, stmt *sqlparser.Select) ([]Column,
 		switch expr := item.(type) {
 		case *sqlparser.AliasedExpr:
 			var column Column
-			var alias string = expr.As.String()
+			var alias = expr.As.String()
 			switch colExpr := expr.Expr.(type) {
 			case *sqlparser.ColName:
 				fieldName := colExpr.Name.String()

sql/model.go

@@ -13,6 +13,7 @@ var (
 	ErrUnsupportedQuery = errors.New("unsupported query")
 )
 
+// KeyIndexColumn is a column for a keyed index.
 // TODO: what the difference between this and IDIndexColumn?
 type KeyIndexColumn struct {
 	Index *pilosa.Index

sql3/parser/walk.go

@@ -1,9 +1,10 @@
 // Copyright 2021 Molecula Corp. All rights reserved.
 package parser
 
-// A Visitor's Visit method is invoked for each node encountered by Walk.
-// If the result visitor w is not nil, Walk visits each of the children
-// of node with the visitor w, followed by a call of w.Visit(nil).
+// Visitor is an interface implemented by anything needed to act on nodes walked
+// during a node traversal. A Visitor's Visit method is invoked for each node
+// encountered by Walk. If the result visitor w is not nil, Walk visits each of
+// the children of node with the visitor w, followed by a call of w.Visit(nil).
 type Visitor interface {
 	Visit(node Node) (w Visitor, n Node, err error)
 	VisitEnd(node Node) (Node, error)

sql3/planner/planoptimizer.go

@@ -21,7 +21,7 @@ import (
 
 //TODO(pok) push down filters thru subqueries with aliases
 
-// a function prototype for all optimizer rules
+// OptimizerFunc is a function prototype for all optimizer rules.
 type OptimizerFunc func(context.Context, *ExecutionPlanner, types.PlanOperator, *OptimizerScope) (types.PlanOperator, bool, error)
 
 // a list of optimzer rules; order can be important important
@@ -62,8 +62,9 @@ var optimizerFunctions = []OptimizerFunc{
 	pushdownPQLTop,
 }
 
-// this will be used in future for symbol resolution when CTEs and subquery support matures
-// and we need to introduce the concept of scope to symbol resolution
+// OptimizerScope will be used in future for symbol resolution when CTEs and
+// subquery support matures and we need to introduce the concept of scope to
+// symbol resolution.
 type OptimizerScope struct {
 }
 

sql3/planner/planwalker.go

@@ -276,9 +276,9 @@ type ExprWithPlanOpFunc func(op types.PlanOperator, expr types.PlanExpression) (
 // whether the expression was modified, and an error or nil.
 type ExprFunc func(expr types.PlanExpression) (types.PlanExpression, bool, error)
 
-// ExprExprSelectorFunc is a function that can be used as a filter selector during
-// expression transformation - it is called before calling a transformation function on
-// a child expression; if it returns false, the child is skipped
+// ExprSelectorFunc is a function that can be used as a filter selector during
+// expression transformation - it is called before calling a transformation
+// function on a child expression; if it returns false, the child is skipped
 type ExprSelectorFunc func(parentExpr, childExpr types.PlanExpression) bool
 
 // TransformSinglePlanOpExpressions applies a transformation function to all expressions on the given plan operator

sql3/planner/types/planexpression.go

@@ -28,14 +28,14 @@ type PlanExpression interface {
 	Plan() map[string]interface{}
 }
 
-// Aggregation buffer is an interface to something that maintains an aggregate during query
-// execution
+// AggregationBuffer is an interface to something that maintains an aggregate
+// during query execution.
 type AggregationBuffer interface {
 	Eval(ctx context.Context) (interface{}, error)
 	Update(ctx context.Context, row Row) error
 }
 
-// interface to an expression that is a an aggregate
+// Aggregable is an interface for an expression that is a an aggregate.
 type Aggregable interface {
 	fmt.Stringer
 
@@ -52,7 +52,8 @@ type Aggregable interface {
 	Type() parser.ExprDataType
 }
 
-// interface to something that can be identified by a name
+// IdentifiableByName is an interface for something that can be identified by a
+// name.
 type IdentifiableByName interface {
 	Name() string
 }

test/pilosa.go

@@ -20,7 +20,6 @@ import (
 	"github.com/featurebasedb/featurebase/v3/server"
 )
 
-// //////////////////////////////////////////////////////////////////////////////////
 // Command represents a test wrapper for server.Command.
 type Command struct {
 	*server.Command

testhook/auditor.go

@@ -84,11 +84,11 @@ func (*NopAuditor) Registry(interface{}) (Registry, error) {
 	return NewNopRegistry(), nil
 }
 
-func (*NopAuditor) Check() (error, []error) {
+func (*NopAuditor) Check() (error, []error) { //nolint:stylecheck
 	return nil, nil
 }
 
-func (*NopAuditor) FinalCheck() (error, []error) {
+func (*NopAuditor) FinalCheck() (error, []error) { //nolint:stylecheck
 	return nil, nil
 }
 
@@ -116,11 +116,11 @@ func (v *VerifyCloseAuditor) Registry(o interface{}) (Registry, error) {
 	return reg, nil
 }
 
-func (*VerifyCloseAuditor) Check() (error, []error) {
+func (*VerifyCloseAuditor) Check() (error, []error) { //nolint:stylecheck
 	return nil, nil
 }
 
-func (v *VerifyCloseAuditor) FinalCheck() (error, []error) {
+func (v *VerifyCloseAuditor) FinalCheck() (error, []error) { //nolint:stylecheck
 	v.regMu.Lock()
 	defer v.regMu.Unlock()
 	var errs []error

testhook/registry.go

@@ -80,6 +80,8 @@ type RegistryHookPostDestroy interface {
 	WasDestroyed(interface{}, KV, *RegistryEntry, error) error
 }
 
+// RegistryHookLive is an interface implemented by a registry hook.
+//
 // If a registry's hooks implement RegistryHookLive, Live() should
 // return only those entries for which a non-nil error was returned, with the
 // error inserted in the RegistryEntry.

vprint/vprint.go

@@ -16,7 +16,7 @@ import (
 const RFC3339MsecTz0 = "2006-01-02T15:04:05.000Z07:00"
 const RFC3339UsecTz0 = "2006-01-02T15:04:05.000000Z07:00"
 
-// for tons of debug output
+// VerboseVerbose will result in tons of debug output when set to true.
 var VerboseVerbose bool = false
 
 func init() {
@@ -73,7 +73,7 @@ func ts() string {
 	return time.Now().Format(RFC3339UsecTz0)
 }
 
-// so we can multi write easily, use our own printf
+// OurStdout is used so we can multi write easily, use our own printf.
 var OurStdout io.Writer = os.Stdout
 
 // Printf formats according to a format specifier and writes to standard output.