From 258646a7d5d3b2edf1a110550fd6525a9380ba55 Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Fri, 1 Feb 2019 20:44:49 -0600 Subject: [PATCH 01/19] Add golangci-lint to Makefile and CI config --- .circleci/config.yml | 11 +++++++++++ Makefile | 9 ++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 677f1a57c..477f7f230 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -35,6 +35,12 @@ jobs: steps: - *fast-checkout - run: make check-license-headers + golangci-lint: + <<: *defaults + steps: + - *fast-checkout + - run: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.13.2 + - run: make golangci-lint test-build-arm: <<: *defaults steps: @@ -131,6 +137,9 @@ workflows: - check-license-headers: requires: - setup + - golangci-lint: + requires: + - setup - test-build-arm: requires: - setup @@ -158,6 +167,7 @@ workflows: requires: - linter - check-license-headers + - golangci-lint - test-golang-1.12 filters: tags: @@ -172,3 +182,4 @@ workflows: - linter - check-license-headers - test-golang-1.12 + - golangci-lint diff --git a/Makefile b/Makefile index 3fa415ccb..b3604dca9 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build check-clean clean cover cover-viz default docker docker-build docker-test generate generate-protoc generate-pql gometalinter install install-build-deps install-gometalinter install-protoc install-protoc-gen-gofast install-peg prerelease prerelease-upload release release-build test +.PHONY: build check-clean clean cover cover-viz default docker docker-build docker-test generate generate-protoc generate-pql gometalinter install install-build-deps install-golangci-lint install-gometalinter install-protoc install-protoc-gen-gofast install-peg prerelease prerelease-upload release release-build test CLONE_URL=github.com/pilosa/pilosa VERSION := $(shell git describe --tags 2> /dev/null || echo unknown) @@ -131,6 +131,10 @@ docker-build: docker-test: docker run --rm -v $(PWD):/go/src/$(CLONE_URL) -w /go/src/$(CLONE_URL) golang:$(GO_VERSION) go test -tags='$(BUILD_TAGS)' $(TESTFLAGS) ./... +# Run golangci-lint +golangci-lint: require-golangci-lint + golangci-lint run + # Run gometalinter with custom flags gometalinter: require-gometalinter vendor GO111MODULE=off gometalinter --vendor --disable-all \ @@ -185,6 +189,9 @@ install-protoc: install-peg: GO111MODULE=off go get github.com/pointlander/peg +install-golangci-lint: + go get -u github.com/golangci/golangci-lint/cmd/golangci-lint + install-gometalinter: GO111MODULE=off go get -u github.com/alecthomas/gometalinter GO111MODULE=off gometalinter --install From d84fcb09e7293617f866d4bd33f5bcd9f7231569 Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Fri, 1 Feb 2019 21:10:32 -0600 Subject: [PATCH 02/19] Workaround due to write permission to bin directory Pro tip: If you're gonna curl|bash, at least don't curl|sudo bash. --- .circleci/config.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 477f7f230..eb47ffcf8 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -39,7 +39,8 @@ jobs: <<: *defaults steps: - *fast-checkout - - run: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.13.2 + - run: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s v1.13.2 + - run: sudo cp bin/golangci-lint /usr/local/bin/ - run: make golangci-lint test-build-arm: <<: *defaults From d49172446147a7fad663c9f02aab0c904410fbfa Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 29 Mar 2019 12:14:39 -0500 Subject: [PATCH 03/19] don't go get -u for golangci-lint golangci-lint is actually dependent on a specific not-quite most recent version of golang.org/x/tools, fixing the dependency is hard and requires changing one of the upstream packages, just omitting the `-u` lets golangci-lint grab the version it wants and use that. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index b3604dca9..52839b3fe 100644 --- a/Makefile +++ b/Makefile @@ -190,7 +190,7 @@ install-peg: GO111MODULE=off go get github.com/pointlander/peg install-golangci-lint: - go get -u github.com/golangci/golangci-lint/cmd/golangci-lint + go get github.com/golangci/golangci-lint/cmd/golangci-lint install-gometalinter: GO111MODULE=off go get -u github.com/alecthomas/gometalinter From 3070c2d4ac4b4f041df991646f8de614da610f74 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 1 Apr 2019 11:27:44 -0500 Subject: [PATCH 04/19] try suppressing modules for golangci-lint --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 52839b3fe..9757af157 100644 --- a/Makefile +++ b/Makefile @@ -190,7 +190,7 @@ install-peg: GO111MODULE=off go get github.com/pointlander/peg install-golangci-lint: - go get github.com/golangci/golangci-lint/cmd/golangci-lint + GO111MODULE=off go get github.com/golangci/golangci-lint/cmd/golangci-lint install-gometalinter: GO111MODULE=off go get -u github.com/alecthomas/gometalinter From 20a8c48552a878514533a16b6f8cf420d0d441ec Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 29 Mar 2019 12:22:58 -0500 Subject: [PATCH 05/19] boltdb/attrstore.go: fix up lint about error checking There's two kinds of unchecked errors here. Writes to a hash (we don't care, hash functions usually don't error in ways we care about), and rollbacks of non-writing transactions to a database. After studying the boltdb docs, I concluded that the recommended solution is to use the `.View(...)` function instead of directly controlling the transaction, so I switched the functions to do that. --- boltdb/attrstore.go | 92 ++++++++++++++++++++++----------------------- 1 file changed, 45 insertions(+), 47 deletions(-) diff --git a/boltdb/attrstore.go b/boltdb/attrstore.go index 260892646..280275a4d 100644 --- a/boltdb/attrstore.go +++ b/boltdb/attrstore.go @@ -215,66 +215,64 @@ func (s *attrStore) SetBulkAttrs(m map[uint64]map[string]interface{}) error { } // Blocks returns a list of all blocks in the store. -func (s *attrStore) Blocks() ([]pilosa.AttrBlock, error) { - tx, err := s.db.Begin(false) - if err != nil { - return nil, errors.Wrap(err, "starting transaction") - } - defer tx.Rollback() +func (s *attrStore) Blocks() (blocks []pilosa.AttrBlock, err error) { + err = s.db.View(func(tx *bolt.Tx) error { + // Wrap cursor to segment by block. + cur := newBlockCursor(tx.Bucket([]byte("attrs")).Cursor(), attrBlockSize) - // Wrap cursor to segment by block. - cur := newBlockCursor(tx.Bucket([]byte("attrs")).Cursor(), attrBlockSize) + // Iterate over each block. + for cur.nextBlock() { + block := pilosa.AttrBlock{ID: cur.blockID()} - // Iterate over each block. - var blocks []pilosa.AttrBlock - for cur.nextBlock() { - block := pilosa.AttrBlock{ID: cur.blockID()} + // Compute checksum of every key/value in block. + h := xxhash.New() + for k, v := cur.next(); k != nil; k, v = cur.next() { + // hash function writes don't usually need to be checked + _, _ = h.Write(k) + _, _ = h.Write(v) + } + block.Checksum = h.Sum(nil) - // Compute checksum of every key/value in block. - h := xxhash.New() - for k, v := cur.next(); k != nil; k, v = cur.next() { - h.Write(k) - h.Write(v) + // Append block. + blocks = append(blocks, block) } - block.Checksum = h.Sum(nil) - - // Append block. - blocks = append(blocks, block) + return nil + }) + if err != nil { + return nil, err } - return blocks, nil } // BlockData returns all data for a single block. -func (s *attrStore) BlockData(i uint64) (map[uint64]map[string]interface{}, error) { - m := make(map[uint64]map[string]interface{}) +func (s *attrStore) BlockData(i uint64) (m map[uint64]map[string]interface{}, err error) { + m = make(map[uint64]map[string]interface{}) // Start read-only transaction. - tx, err := s.db.Begin(false) + err = s.db.View(func(tx *bolt.Tx) error { + // Move to the start of the block. + min := u64tob(i * attrBlockSize) + max := u64tob((i + 1) * attrBlockSize) + cur := tx.Bucket([]byte("attrs")).Cursor() + for k, v := cur.Seek(min); k != nil; k, v = cur.Next() { + // Exit if we're past the end of the block. + if bytes.Compare(k, max) != -1 { + break + } + + // Decode attribute map and associate with id. + attrs, err := pilosa.DecodeAttrs(v) + if err != nil { + return errors.Wrap(err, "decoding attrs") + } + m[btou64(k)] = attrs + + } + return nil + }) if err != nil { - return nil, errors.Wrap(err, "starting transaction") + return nil, err } - defer tx.Rollback() - - // Move to the start of the block. - min := u64tob(i * attrBlockSize) - max := u64tob((i + 1) * attrBlockSize) - cur := tx.Bucket([]byte("attrs")).Cursor() - for k, v := cur.Seek(min); k != nil; k, v = cur.Next() { - // Exit if we're past the end of the block. - if bytes.Compare(k, max) != -1 { - break - } - - // Decode attribute map and associate with id. - attrs, err := pilosa.DecodeAttrs(v) - if err != nil { - return nil, errors.Wrap(err, "decoding attrs") - } - m[btou64(k)] = attrs - - } - return m, nil } From 77d49ded6494999f207e4ed90e12f4374f9bef1b Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 29 Mar 2019 14:40:21 -0500 Subject: [PATCH 06/19] so much lint So with the switch to a new linter, we get a lot of new warnings, and the majority of them are harmless probably, but a few might be real. Variously just use _ to suppress warnings, or report errors. There's probably things here that deserve better fixes, but we can always revisit it. --- cluster.go | 11 +- cluster_internal_test.go | 68 ++++++++--- cmd/root.go | 3 +- cmd/root_test.go | 6 +- ctl/check.go | 15 ++- ctl/check_test.go | 16 ++- ctl/config_test.go | 13 +- ctl/export_test.go | 12 +- ctl/generate_config_test.go | 12 +- ctl/import_test.go | 194 +++++++++++++++++++++++------- ctl/inspect.go | 8 +- ctl/inspect_test.go | 13 +- diagnostics_internal_test.go | 21 +++- enterprise/b/btree.go | 4 +- enterprise/b/containers_btree.go | 3 +- field.go | 4 +- field_internal_test.go | 10 +- field_test.go | 15 ++- fragment.go | 4 +- fragment_internal_test.go | 89 +++++++++++--- gopsutil/systeminfo.go | 4 +- holder.go | 8 +- holder_internal_test.go | 46 +++++-- holder_test.go | 14 ++- http/client.go | 5 +- http/client_test.go | 15 ++- http/handler.go | 45 +++++-- logger/logger.go | 23 ++++ lru/lru.go | 4 +- pql/pql.peg.go | 2 +- roaring/btree.go | 10 +- roaring/containers_btree.go | 3 +- roaring/roaring.go | 13 +- roaring/roaring_internal_test.go | 55 +++++++-- roaring/roaring_test.go | 199 +++++++++++++++++++------------ server.go | 7 +- server/handler_test.go | 10 +- server/server_test.go | 7 +- utils_internal_test.go | 6 +- view.go | 2 +- 40 files changed, 733 insertions(+), 266 deletions(-) diff --git a/cluster.go b/cluster.go index 204a1016b..024c426a1 100644 --- a/cluster.go +++ b/cluster.go @@ -844,8 +844,8 @@ func (c *cluster) partition(index string, shard uint64) int { // Hash the bytes and mod by partition count. h := fnv.New64a() - h.Write([]byte(index)) - h.Write(buf[:]) + _, _ = h.Write([]byte(index)) + _, _ = h.Write(buf[:]) return int(h.Sum64() % uint64(c.partitionN)) } @@ -1892,7 +1892,12 @@ func (c *cluster) mergeClusterStatus(cs *ClusterStatus) error { for _, node := range officialNodes { if node.ID == c.Node.ID && node.State != c.Node.State { c.logger.Printf("mismatched state in mergeClusterStatus got %v have %v", node.State, c.Node.State) - go c.setNodeState(c.Node.State) + go func() { + err := c.setNodeState(c.Node.State) + if err != nil { + c.logger.Printf("error setting node state from %v to %v: %v", node.State, c.Node.State, err) + } + }() } if err := c.addNode(node); err != nil { return errors.Wrap(err, "adding node") diff --git a/cluster_internal_test.go b/cluster_internal_test.go index fa40c48b7..84392ffd9 100644 --- a/cluster_internal_test.go +++ b/cluster_internal_test.go @@ -607,7 +607,9 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Single node, in topology", func(t *testing.T) { tc := NewClusterCluster(0) - tc.addNode() + if err := tc.addNode(); err != nil { + t.Fatalf("adding node: %v", err) + } node := tc.Clusters[0] @@ -615,7 +617,9 @@ func TestCluster_ResizeStates(t *testing.T) { top := &Topology{ nodeIDs: []string{node.Node.ID}, } - tc.WriteTopology(node.Path, top) + if err := tc.WriteTopology(node.Path, top); err != nil { + t.Fatalf("writing topology: %v", err) + } // Open TestCluster. if err := tc.Open(); err != nil { @@ -635,7 +639,9 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Single node, not in topology", func(t *testing.T) { tc := NewClusterCluster(0) - tc.addNode() + if err := tc.addNode(); err != nil { + t.Fatalf("adding node: %v", err) + } node := tc.Clusters[0] @@ -643,7 +649,9 @@ func TestCluster_ResizeStates(t *testing.T) { top := &Topology{ nodeIDs: []string{"some-other-host"}, } - tc.WriteTopology(node.Path, top) + if err := tc.WriteTopology(node.Path, top); err != nil { + t.Fatalf("writing topology: %v", err) + } // Open TestCluster. expected := "coordinator node0 is not in topology: [some-other-host]" @@ -660,14 +668,18 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Multiple nodes, no data", func(t *testing.T) { tc := NewClusterCluster(0) - tc.addNode() + if err := tc.addNode(); err != nil { + t.Fatalf("adding node: %v", err) + } // Open TestCluster. if err := tc.Open(); err != nil { - t.Fatal(err) + t.Fatalf("opening cluster: %v", err) } - tc.addNode() + if err := tc.addNode(); err != nil { + t.Fatalf("adding node: %v", err) + } node0 := tc.Clusters[0] node1 := tc.Clusters[1] @@ -698,18 +710,23 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Multiple nodes, in/not in topology", func(t *testing.T) { tc := NewClusterCluster(0) - tc.addNode() + err := tc.addNode() + if err != nil { + t.Fatalf("adding node: %v", err) + } node0 := tc.Clusters[0] // write topology to data file top := &Topology{ nodeIDs: []string{"node0", "node2"}, } - tc.WriteTopology(node0.Path, top) + if err := tc.WriteTopology(node0.Path, top); err != nil { + t.Fatalf("writing topology: %v", err) + } // Open TestCluster. if err := tc.Open(); err != nil { - t.Fatal(err) + t.Fatalf("opening cluster: %v", err) } // Ensure that node is in state STARTING before the other node joins. @@ -719,19 +736,22 @@ func TestCluster_ResizeStates(t *testing.T) { // Expect an error by adding a node not in the topology. expectedError := "host is not in topology: node1" - err := tc.addNode() + err = tc.addNode() if err == nil || err.Error() != expectedError { t.Errorf("did not receive expected error: %s", expectedError) } - tc.addNode() + err = tc.addNode() + if err != nil { + t.Fatalf("adding node: %v", err) + } node2 := tc.Clusters[2] // Ensure that node comes up in state NORMAL. if node0.State() != ClusterStateNormal { t.Errorf("expected node0 state: %v, but got: %v", ClusterStateNormal, node0.State()) } else if node2.State() != ClusterStateNormal { - t.Errorf("expected node1 state: %v, but got: %v", ClusterStateNormal, node2.State()) + t.Errorf("expected node2 state: %v, but got: %v", ClusterStateNormal, node2.State()) } // Close TestCluster. @@ -742,20 +762,27 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Multiple nodes, with data", func(t *testing.T) { tc := NewClusterCluster(0) - tc.addNode() + err := tc.addNode() + if err != nil { + t.Fatalf("adding node: %v", err) + } node0 := tc.Clusters[0] // Open TestCluster. - if err := tc.Open(); err != nil { + if err = tc.Open(); err != nil { t.Fatal(err) } // Add Bit Data to node0. if err := tc.CreateField("i", "f", OptFieldTypeDefault()); err != nil { - t.Fatal(err) + t.Fatalf("creating field: %v", err) + } + if err := tc.SetBit("i", "f", 1, 101, nil); err != nil { + t.Fatalf("setting bit: %v", err) + } + if err := tc.SetBit("i", "f", 1, ShardWidth+1, nil); err != nil { + t.Fatalf("setting bit: %v", err) } - tc.SetBit("i", "f", 1, 101, nil) - tc.SetBit("i", "f", 1, ShardWidth+1, nil) // Before starting the resize, get the CheckSum to use for // comparison later. @@ -765,7 +792,10 @@ func TestCluster_ResizeStates(t *testing.T) { node0Checksum := node0Fragment.Checksum() // addNode needs to block until the resize process has completed. - tc.addNode() + err = tc.addNode() + if err != nil { + t.Fatalf("adding node: %v", err) + } node1 := tc.Clusters[1] // Ensure that nodes come up in state NORMAL. diff --git a/cmd/root.go b/cmd/root.go index 64b8b1ca9..a8f8e6ddc 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -54,9 +54,8 @@ Build Time: ` + pilosa.BuildTime + "\n", if ret, err := cmd.Flags().GetBool("dry-run"); ret && err == nil { if cmd.Parent() != nil { return fmt.Errorf("dry run") - } else if err != nil { - return fmt.Errorf("problem getting dry-run flag: %v", err) } + return fmt.Errorf("problem getting dry-run flag: %v", err) } return nil diff --git a/cmd/root_test.go b/cmd/root_test.go index 28bbea20d..6807aaf3c 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -187,10 +187,12 @@ bind = "127.0.0.1:10101" "127.0.0.1:10101", "127.0.0.1:10111", ]` - file.Write([]byte(config)) + if _, err := file.Write([]byte(config)); err != nil { + t.Fatalf("writing config file: %v", err) + } file.Close() _, err = ExecNewRootCommand(t, "server", "--config", file.Name()) - if err.Error() != "invalid option in configuration file: cluster.partitions" { + if err == nil || err.Error() != "invalid option in configuration file: cluster.partitions" { t.Fatalf("Expected invalid option in configuration file, but err: '%v'", err) } } diff --git a/ctl/check.go b/ctl/check.go index b1389b6ee..0beee5bc5 100644 --- a/ctl/check.go +++ b/ctl/check.go @@ -68,7 +68,7 @@ func (cmd *CheckCommand) Run(_ context.Context) error { } // checkBitmapFile performs a consistency check on path for a roaring bitmap file. -func (cmd *CheckCommand) checkBitmapFile(path string) error { +func (cmd *CheckCommand) checkBitmapFile(path string) (err error) { // Open file handle. f, err := os.Open(path) if err != nil { @@ -86,8 +86,17 @@ func (cmd *CheckCommand) checkBitmapFile(path string) error { if err != nil { return errors.Wrap(err, "mmapping") } - defer syscall.Munmap(data) - + defer func() { + e := syscall.Munmap(data) + if e != nil { + fmt.Fprintf(cmd.Stderr, "WARNING: munmap failed: %v", e) + } + // don't overwrite another error with this, but also indicate + // this error. + if err == nil { + err = e + } + }() // Attach the mmap file to the bitmap. bm := roaring.NewBitmap() if err := bm.UnmarshalBinary(data); err != nil { diff --git a/ctl/check_test.go b/ctl/check_test.go index 1b2ebd6f6..e37d99358 100644 --- a/ctl/check_test.go +++ b/ctl/check_test.go @@ -40,7 +40,9 @@ func TestCheckCommand_RunCacheFile(t *testing.T) { err := cm.Run(context.Background()) w.Close() var buf bytes.Buffer - io.Copy(&buf, r) + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("copy: %v", err) + } if !strings.Contains(buf.String(), "ignoring cache file") { t.Fatalf("expect: ignoring cache file, actual: '%s'", err) @@ -59,7 +61,9 @@ func TestCheckCommand_RunSnapshot(t *testing.T) { err := cm.Run(context.Background()) w.Close() var buf bytes.Buffer - io.Copy(&buf, r) + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("copy: %v", err) + } if !strings.Contains(buf.String(), "ignoring snapshot file") { t.Fatalf("expect: ignoring snapshot file, actual: '%s'", err) @@ -71,7 +75,9 @@ func TestCheckCommand_Run(t *testing.T) { if err != nil { t.Fatal(err) } - file.Write([]byte("1234,1223")) + if _, err := file.Write([]byte("1234,1223")); err != nil { + t.Fatalf("writing to temp file: %v", err) + } file.Close() rder := []byte{} @@ -83,7 +89,9 @@ func TestCheckCommand_Run(t *testing.T) { err = cm.Run(context.Background()) w.Close() var buf bytes.Buffer - io.Copy(&buf, r) + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("copy: %v", err) + } if !strings.HasPrefix(err.Error(), "checking bitmap: unmarshalling: reading roaring header:") { t.Fatalf("expect error: invalid roaring file, actual: '%s'", err) diff --git a/ctl/config_test.go b/ctl/config_test.go index ca08c273b..b9a5dc0b5 100644 --- a/ctl/config_test.go +++ b/ctl/config_test.go @@ -33,13 +33,16 @@ func TestConfigCommand_Run(t *testing.T) { cm.Config = server.NewConfig() err := cm.Run(context.Background()) - w.Close() - var buf bytes.Buffer - io.Copy(&buf, r) - if err != nil { t.Fatalf("Config Run doesn't work: %s", err) - } else if !strings.Contains(buf.String(), ":10101") { + } + w.Close() + var buf bytes.Buffer + _, err = io.Copy(&buf, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(buf.String(), ":10101") { t.Fatalf("Unexpected config: \n%s", buf.String()) } } diff --git a/ctl/export_test.go b/ctl/export_test.go index e3189efe3..6cb611460 100644 --- a/ctl/export_test.go +++ b/ctl/export_test.go @@ -52,8 +52,16 @@ func TestExportCommand_Run(t *testing.T) { hostport := cmd.API.Node().URI.HostPort() cm.Host = hostport - http.DefaultClient.Do(test.MustNewHTTPRequest("POST", "http://"+hostport+"/index/i", strings.NewReader(""))) - http.DefaultClient.Do(test.MustNewHTTPRequest("POST", "http://"+hostport+"/index/i/field/f", strings.NewReader(""))) + resp, err := http.DefaultClient.Do(test.MustNewHTTPRequest("POST", "http://"+hostport+"/index/i", strings.NewReader(""))) + if err != nil { + t.Fatalf("making http request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(test.MustNewHTTPRequest("POST", "http://"+hostport+"/index/i/field/f", strings.NewReader(""))) + if err != nil { + t.Fatalf("making http request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" diff --git a/ctl/generate_config_test.go b/ctl/generate_config_test.go index 26b531e8f..a431cd468 100644 --- a/ctl/generate_config_test.go +++ b/ctl/generate_config_test.go @@ -29,12 +29,16 @@ func TestGenerateConfigCommand_Run(t *testing.T) { r, w, _ := os.Pipe() cm := NewGenerateConfigCommand(stdin, w, os.Stderr) err := cm.Run(context.Background()) - w.Close() - var buf bytes.Buffer - io.Copy(&buf, r) if err != nil { t.Fatalf("Config Run doesn't work: %s", err) - } else if !strings.Contains(buf.String(), ":10101") { + } + w.Close() + var buf bytes.Buffer + _, err = io.Copy(&buf, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(buf.String(), ":10101") { t.Fatalf("Unexpected config: %s", buf.String()) } } diff --git a/ctl/import_test.go b/ctl/import_test.go index e701af1b2..b20919203 100644 --- a/ctl/import_test.go +++ b/ctl/import_test.go @@ -58,7 +58,13 @@ func TestImportCommand_Basic(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import.csv") - file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("creating tempfile: %v", err) + } + _, err = file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } ctx := context.Background() if err != nil { t.Fatal(err) @@ -82,11 +88,14 @@ func TestImportCommand_Basic(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import.csv") - file.Write([]byte("1,2\n3,4\n5,6")) - ctx := context.Background() if err != nil { - t.Fatal(err) + t.Fatalf("creating tempfile: %v", err) } + _, err = file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } + ctx := context.Background() cmd := test.MustRunCluster(t, 1)[0] cm.Host = cmd.API.Node().URI.HostPort() @@ -110,17 +119,28 @@ func TestImportCommand_RunValue(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import-value.csv") - file.Write([]byte("1,2\n3,4\n5,6")) - ctx := context.Background() if err != nil { - t.Fatal(err) + t.Fatalf("creating tempfile: %v", err) } + _, err = file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } + ctx := context.Background() cmd := test.MustRunCluster(t, 1)[0] cm.Host = cmd.API.Node().URI.HostPort() - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max": 100}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) + if err != nil { + t.Fatalf("http request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max": 100}}`))) + if err != nil { + t.Fatalf("http request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -136,7 +156,13 @@ func TestImportCommand_RunValue(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import-value.csv") - file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("creating tempfile: %v", err) + } + _, err = file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } ctx := context.Background() if err != nil { t.Fatal(err) @@ -145,8 +171,16 @@ func TestImportCommand_RunValue(t *testing.T) { cmd := test.MustRunCluster(t, 1)[0] cm.Host = cmd.API.Node().URI.HostPort() - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max": 100}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max": 100}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -165,17 +199,28 @@ func TestImportCommand_RunKeys(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import-key.csv") - file.Write([]byte("foo1,bar2\nfoo3,bar4\nfoo5,bar6")) - ctx := context.Background() if err != nil { t.Fatal(err) } + _, err = file.Write([]byte("foo1,bar2\nfoo3,bar4\nfoo5,bar6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } + ctx := context.Background() cmd := test.MustRunCluster(t, 1)[0] cm.Host = cmd.API.Node().URI.HostPort() - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(`{"options":{"keys": true}}`))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"keys": true}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(`{"options":{"keys": true}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"keys": true}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -192,7 +237,9 @@ func TestImportCommand_KeyReplication(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import-key.csv") - + if err != nil { + t.Fatal(err) + } // create a large import file in order to test the // translateStoreBufferSize growth logic. keyBytes := []byte{} @@ -205,11 +252,11 @@ func TestImportCommand_KeyReplication(t *testing.T) { x := "fooEND,barEND" keyBytes = append(keyBytes, x...) - file.Write(keyBytes) - ctx := context.Background() + _, err = file.Write(keyBytes) if err != nil { - t.Fatal(err) + t.Fatalf("writing to tempfile: %v", err) } + ctx := context.Background() c := test.MustRunCluster(t, 2) cmd0 := c[0] @@ -220,8 +267,16 @@ func TestImportCommand_KeyReplication(t *testing.T) { cm.Host = host0 - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(`{"options":{"keys": true}}`))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"keys": true}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(`{"options":{"keys": true}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"keys": true}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -255,17 +310,28 @@ func TestImportCommand_RunValueKeys(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import-key.csv") - file.Write([]byte("foo1,2\nfoo3,4\nfoo5,6")) - ctx := context.Background() if err != nil { t.Fatal(err) } + _, err = file.Write([]byte("foo1,bar2\nfoo3,bar4\nfoo5,bar6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } + ctx := context.Background() cmd := test.MustRunCluster(t, 1)[0] cm.Host = cmd.API.Node().URI.HostPort() - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(`{"options":{"keys": true}}`))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max": 100}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(`{"options":{"keys": true}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max": 100}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -286,9 +352,12 @@ func TestImportCommand_InvalidFile(t *testing.T) { cm.Index = "i" cm.Field = "f" file, err := ioutil.TempFile("", "import.csv") - file.Write([]byte("a,2\n3,5\n5,6")) if err != nil { - t.Fatal(err) + t.Fatalf("creating tempfile: %v", err) + } + _, err = file.Write([]byte("1,2\n3,4\n5,6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) } cm.Paths = []string{file.Name()} err = cm.Run(context.Background()) @@ -297,9 +366,12 @@ func TestImportCommand_InvalidFile(t *testing.T) { } file, err = ioutil.TempFile("", "import1.csv") - file.Write([]byte("1,\n3,\n5,6")) if err != nil { - t.Fatal(err) + t.Fatalf("creating tempfile: %v", err) + } + _, err = file.Write([]byte("1,\n3,\n5,6")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) } cm.Paths = []string{file.Name()} err = cm.Run(context.Background()) @@ -308,7 +380,10 @@ func TestImportCommand_InvalidFile(t *testing.T) { } file, err = ioutil.TempFile("", "import1.csv") - file.Write([]byte("1,2,34343\n1,3,54565,\n5,6,565")) + if err != nil { + t.Fatal(err) + } + _, err = file.Write([]byte("1,2,34343\n1,3,54565,\n5,6,565")) if err != nil { t.Fatal(err) } @@ -319,7 +394,10 @@ func TestImportCommand_InvalidFile(t *testing.T) { } file, err = ioutil.TempFile("", "import1.csv") - file.Write([]byte("1\n3\n5")) + if err != nil { + t.Fatal(err) + } + _, err = file.Write([]byte("1\n3\n5")) if err != nil { t.Fatal(err) } @@ -357,16 +435,27 @@ func TestImportCommand_BugOverwriteValue(t *testing.T) { stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) file, err := ioutil.TempFile("", "import-value.csv") - file.Write([]byte("0,17\n")) - ctx := context.Background() if err != nil { t.Fatal(err) } + _, err = file.Write([]byte("0,17\n")) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() cm.Host = cmd.API.Node().URI.HostPort() - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max":2147483648 }}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "int", "min": 0, "max":2147483648 }}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -381,7 +470,10 @@ func TestImportCommand_BugOverwriteValue(t *testing.T) { if err != nil { t.Fatalf("Error creating tempfile: %s", err) } - file.Write([]byte("0,16\n")) + _, err = file.Write([]byte("0,16\n")) + if err != nil { + t.Fatalf("writing bytes to tempfile: %v", err) + } cm.Paths = []string{file.Name()} err = cm.Run(ctx) if err != nil { @@ -393,7 +485,10 @@ func TestImportCommand_BugOverwriteValue(t *testing.T) { if err != nil { t.Fatalf("Error creating tempfile: %s", err) } - file.Write([]byte("0,19\n")) + _, err = file.Write([]byte("0,19\n")) + if err != nil { + t.Fatalf("writing bytes to tempfile: %v", err) + } cm.Paths = []string{file.Name()} err = cm.Run(ctx) if err != nil { @@ -411,8 +506,16 @@ func TestImportCommand_RunBool(t *testing.T) { cmd := test.MustRunCluster(t, 1)[0] cm.Host = cmd.API.Node().URI.HostPort() - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) - http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "bool"}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i", strings.NewReader(""))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() + resp, err = http.DefaultClient.Do(MustNewHTTPRequest("POST", "http://"+cm.Host+"/index/i/field/f", strings.NewReader(`{"options":{"type": "bool"}}`))) + if err != nil { + t.Fatalf("posting request: %v", err) + } + resp.Body.Close() cm.Index = "i" cm.Field = "f" @@ -422,7 +525,10 @@ func TestImportCommand_RunBool(t *testing.T) { if err != nil { t.Fatal(err) } - file.Write([]byte("0,1\n1,2\n1,3")) + _, err = file.Write([]byte("0,1\n1,2\n1,3")) + if err != nil { + t.Fatalf("writing bytes to tempfile: %v", err) + } cm.Paths = []string{file.Name()} err = cm.Run(ctx) @@ -437,8 +543,10 @@ func TestImportCommand_RunBool(t *testing.T) { if err != nil { t.Fatal(err) } - file.Write([]byte("0,1\n1,2\n1,3\n2,4")) - + _, err = file.Write([]byte("0,1\n1,2\n1,3\n2,4")) + if err != nil { + t.Fatalf("writing bytes to tempfile: %v", err) + } cm.Paths = []string{file.Name()} err = cm.Run(ctx) if !strings.Contains(err.Error(), "bool field imports only support values 0 and 1") { diff --git a/ctl/inspect.go b/ctl/inspect.go index c0676f8da..204ffab3b 100644 --- a/ctl/inspect.go +++ b/ctl/inspect.go @@ -64,8 +64,12 @@ func (cmd *InspectCommand) Run(_ context.Context) error { if err != nil { return errors.Wrap(err, "mmapping") } - defer syscall.Munmap(data) - + defer func() { + err := syscall.Munmap(data) + if err != nil { + fmt.Fprintf(cmd.Stderr, "inspect command: munmap failed: %v", err) + } + }() // Attach the mmap file to the bitmap. t := time.Now() fmt.Fprintf(cmd.Stderr, "unmarshaling bitmap...") diff --git a/ctl/inspect_test.go b/ctl/inspect_test.go index c7a48d400..265d77a12 100644 --- a/ctl/inspect_test.go +++ b/ctl/inspect_test.go @@ -34,14 +34,23 @@ func TestInspectCommand_Run(t *testing.T) { if err != nil { t.Fatalf("Error creating tempfile: %s", err) } - file.Write([]byte("12358267538963")) + _, err = file.Write([]byte("12358267538963")) + if err != nil { + t.Fatalf("writing to tempfile: %v", err) + } file.Close() cm.Path = file.Name() err = cm.Run(context.Background()) + if err != nil { + t.Fatalf("can't run command: %v", err) + } w.Close() var buf bytes.Buffer - io.Copy(&buf, r) + _, err = io.Copy(&buf, r) + if err != nil { + t.Fatalf("copying data: %v", err) + } if !strings.Contains(buf.String(), "unmarshaling bitmap...") { t.Fatalf("Inspect doesn't work: %s", err) } diff --git a/diagnostics_internal_test.go b/diagnostics_internal_test.go index 99c0a83ac..6ddc98b32 100644 --- a/diagnostics_internal_test.go +++ b/diagnostics_internal_test.go @@ -22,6 +22,8 @@ import ( "runtime" "strings" "testing" + + "github.com/pilosa/pilosa/logger" ) func TestDiagnosticsClient(t *testing.T) { @@ -112,19 +114,34 @@ func TestDiagnosticsVersion_Check(t *testing.T) { // Mock server. server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(versionResponse{ + err := json.NewEncoder(w).Encode(versionResponse{ Version: "1.1.1", }) + if err != nil { + t.Fatalf("couldn't encode version response: %v", err) + } })) // Create a new client. d := newDiagnosticsCollector("localhost:10101") + logs := logger.NewCaptureLogger() + d.Logger = logs + version := "0.1.1" d.SetVersion(version) d.VersionURL = server.URL - d.CheckVersion() + err := d.CheckVersion() + if err != nil { + t.Fatalf("checking version: %v", err) + } + if len(logs.Prints) != 1 { + t.Fatalf("expected a version upgrade message") + } + if !strings.Contains(logs.Prints[0], "a newer version") { + t.Fatalf("expected version upgrade message, got '%s'", logs.Prints[0]) + } } func compareJSON(a, b []byte) (bool, error) { diff --git a/enterprise/b/btree.go b/enterprise/b/btree.go index c61f5c5b6..2fa5c24e8 100644 --- a/enterprise/b/btree.go +++ b/enterprise/b/btree.go @@ -873,7 +873,7 @@ func (e *enumerator) Next() (k uint64, v *roaring.Container, err error) { i := e.q.d[e.i] k, v = i.k, i.v e.k, e.hit = k, true - e.next() + _ = e.next() return k, v, nil } @@ -928,7 +928,7 @@ func (e *enumerator) Prev() (k uint64, v *roaring.Container, err error) { i := e.q.d[e.i] k, v = i.k, i.v e.k, e.hit = k, true - e.prev() + _ = e.prev() return k, v, err } diff --git a/enterprise/b/containers_btree.go b/enterprise/b/containers_btree.go index db5c9946d..a051d7cde 100644 --- a/enterprise/b/containers_btree.go +++ b/enterprise/b/containers_btree.go @@ -44,7 +44,8 @@ func NewBTreeBitmap(a ...uint64) *roaring.Bitmap { b := &roaring.Bitmap{ Containers: newBTreeContainers(), } - b.Add(a...) + // TODO: there's no way to report an error here + _, _ = b.Add(a...) return b } diff --git a/field.go b/field.go index bd4dd1aca..d83735893 100644 --- a/field.go +++ b/field.go @@ -612,7 +612,9 @@ func (f *Field) createBSIGroup(bsig *bsiGroup) error { if err := f.addBSIGroup(bsig); err != nil { return err } - f.saveMeta() + if err := f.saveMeta(); err != nil { + return errors.Wrap(err, "saving") + } return nil } diff --git a/field_internal_test.go b/field_internal_test.go index 0a26225bb..ad8337c31 100644 --- a/field_internal_test.go +++ b/field_internal_test.go @@ -370,7 +370,10 @@ func TestField_PersistAvailableShardsFootprint(t *testing.T) { // bm represents remote available shards. bm := roaring.NewBitmap() for i := uint64(0); i < 1204; i += 2 { - bm.Add(i) + _, err := bm.Add(i) + if err != nil { + t.Fatalf("adding bits: %v", err) + } } if err := f.AddRemoteAvailableShards(bm); err != nil { @@ -386,7 +389,10 @@ func TestField_PersistAvailableShardsFootprint(t *testing.T) { bm1 := roaring.NewBitmap() for i := uint64(1); i < 1204; i += 2 { - bm1.Add(i) + _, err := bm1.Add(i) + if err != nil { + t.Fatalf("adding bits: %v", err) + } } if err := f.AddRemoteAvailableShards(bm1); err != nil { diff --git a/field_test.go b/field_test.go index 2a0efc0b9..5911abd6a 100644 --- a/field_test.go +++ b/field_test.go @@ -208,17 +208,20 @@ func TestField_AvailableShards(t *testing.T) { } // Set remote shards and verify. - f.AddRemoteAvailableShards(roaring.NewBitmap(1, 2, 4)) + if err := f.AddRemoteAvailableShards(roaring.NewBitmap(1, 2, 4)); err != nil { + t.Fatalf("adding remote shards: %v", err) + } if diff := cmp.Diff(f.AvailableShards().Slice(), []uint64{0, 1, 2, 4}); diff != "" { t.Fatal(diff) } // Delete shards; only local shards should remain. - f.RemoveAvailableShard(0) - f.RemoveAvailableShard(1) - f.RemoveAvailableShard(2) - f.RemoveAvailableShard(3) - f.RemoveAvailableShard(4) + for i := uint64(0); i < 5; i++ { + err := f.RemoveAvailableShard(i) + if err != nil { + t.Fatalf("removing shard: %v", err) + } + } if diff := cmp.Diff(f.AvailableShards().Slice(), []uint64{0, 2}); diff != "" { t.Fatal(diff) } diff --git a/fragment.go b/fragment.go index 82288c914..2409a5fc5 100644 --- a/fragment.go +++ b/fragment.go @@ -1328,7 +1328,7 @@ type topOptions struct { func (f *fragment) Checksum() []byte { h := xxhash.New() for _, block := range f.Blocks() { - h.Write(block.Checksum) + _, _ = h.Write(block.Checksum) } return h.Sum(nil) } @@ -2332,7 +2332,7 @@ func (h *blockHasher) Sum() []byte { func (h *blockHasher) WriteValue(v uint64) { binary.BigEndian.PutUint64(h.buf[:], v) - h.hash.Write(h.buf[:]) + _, _ = h.hash.Write(h.buf[:]) } // fragmentSyncer syncs a local fragment to one on a remote host. diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 3930e98e2..8c3d65c66 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -654,7 +654,9 @@ func TestFragment_Range(t *testing.T) { func benchmarkSetValues(b *testing.B, bitDepth uint, f *fragment, cfunc func(uint64) uint64) { column := uint64(0) for i := 0; i < b.N; i++ { - f.setValue(column, bitDepth, uint64(i)) + // We're not checking the error because this is a benchmark. + // That does mean the result could be completely wrong... + _, _ = f.setValue(column, bitDepth, uint64(i)) column = cfunc(column) } } @@ -943,8 +945,14 @@ func TestFragment_Top_Filter(t *testing.T) { f.mustSetBits(102, 1, 2) f.RecalculateCache() // Assign attributes. - f.RowAttrStore.SetAttrs(101, map[string]interface{}{"x": int64(10)}) - f.RowAttrStore.SetAttrs(102, map[string]interface{}{"x": int64(20)}) + err := f.RowAttrStore.SetAttrs(101, map[string]interface{}{"x": int64(10)}) + if err != nil { + t.Fatalf("setAttrs: %v", err) + } + err = f.RowAttrStore.SetAttrs(102, map[string]interface{}{"x": int64(20)}) + if err != nil { + t.Fatalf("setAttrs: %v", err) + } // Retrieve top rows. if pairs, err := f.top(topOptions{ @@ -2064,7 +2072,10 @@ func BenchmarkImportRoaringUpdateConcurrent(b *testing.B) { for i := 0; i < b.N; i++ { for j := 0; j < concurrency; j++ { frags[j] = mustOpenFragment("i", "f", viewStandard, uint64(j), CacheTypeRanked) - frags[j].importRoaring(data, false) + err := frags[j].importRoaring(data, false) + if err != nil { + b.Fatalf("importing roaring: %v", err) + } } eg := errgroup.Group{} b.StartTimer() @@ -2275,7 +2286,10 @@ func BenchmarkImportRoaringIntoLargeFragment(b *testing.B) { func TestGetZipfRowsSliceRoaring(t *testing.T) { f := mustOpenFragment("i", "f", viewStandard, 0, DefaultCacheType) data := getZipfRowsSliceRoaring(10, 1, 0, ShardWidth) - f.importRoaring(data, false) + err := f.importRoaring(data, false) + if err != nil { + t.Fatalf("importing roaring: %v", err) + } if !reflect.DeepEqual(f.rows(0), []uint64{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}) { t.Fatalf("unexpected rows: %v", f.rows(0)) } @@ -2608,7 +2622,10 @@ func TestFragment_RoaringImport(t *testing.T) { if err != nil { t.Fatalf("writing to buffer: %v", err) } - f.importRoaring(buf.Bytes(), false) + err = f.importRoaring(buf.Bytes(), false) + if err != nil { + t.Fatalf("importing roaring: %v", err) + } exp := calcExpected(test[:num+1]...) for row, expCols := range exp { cols := f.row(uint64(row)).Columns() @@ -2680,7 +2697,10 @@ func TestFragment_RoaringImportTopN(t *testing.T) { if err != nil { t.Fatalf("writing to buffer: %v", err) } - f.importRoaring(buf.Bytes(), false) + err = f.importRoaring(buf.Bytes(), false) + if err != nil { + t.Fatalf("importing roaring: %v", err) + } rows, cols := toRowsCols(test.roaring) expPairs = calcTop(append(test.rowIDs, rows...), append(test.colIDs, cols...)) pairs, err = f.top(topOptions{}) @@ -2895,19 +2915,56 @@ func TestFragmentRowIterator(t *testing.T) { func TestUnionInPlaceMapped(t *testing.T) { f := mustOpenFragment("i", "f", "v", 0, CacheTypeNone) defer f.Clean(t) + // I know this doesn't actually matter in our current context, but + // strictly speaking, we do say you have to hold the lock while calling + // unprotectedWriteToFragment... + f.mu.Lock() + defer f.mu.Unlock() r0 := rand.New(rand.NewSource(2)) r1 := rand.New(rand.NewSource(1)) data0 := randPositions(1000000, r0) - setBM := roaring.NewBitmap() - setBM.OpWriter = nil - setBM.Add(data0...) - unprotectedWriteToFragment(f, setBM) - data1 := randPositions(1000000, r1) - setBM2 := roaring.NewBitmap() - setBM2.OpWriter = nil - setBM2.Add(data1...) + setBM0 := roaring.NewBitmap() + setBM0.OpWriter = nil + _, err := setBM0.Add(data0...) + if err != nil { + t.Fatalf("adding bits: %v", err) + } + count0 := setBM0.Count() - f.storage.UnionInPlace(setBM2) + data1 := randPositions(1000000, r1) + setBM1 := roaring.NewBitmap() + setBM1.OpWriter = nil + _, err = setBM1.Add(data1...) + if err != nil { + t.Fatalf("adding bits: %v", err) + } + count1 := setBM1.Count() + + // now we write setBM0 into f.storage. + err = unprotectedWriteToFragment(f, setBM0) + if err != nil { + t.Fatalf("trying to flush fragment to disk: %v", err) + } + countF := f.storage.Count() + + f.storage.UnionInPlace(setBM1) + countUnion := f.storage.Count() + + if count0 != countF { + t.Fatalf("writing bitmap to storage changed count: %d => %d", count0, countF) + } + min := count0 + if count1 > min { + min = count1 + } + max := count0 + count1 + // We don't know how many bits we should have, because of overlap, + // but it should be between the size of the largest bitmap and the + // sum of the bitmaps. + if countUnion < min || countUnion > max { + t.Fatalf("union of sets with cardinality %d and %d should be between %d and %d, got %d", + count0, count1, min, max, countUnion) + } } func randPositions(n int, r *rand.Rand) []uint64 { diff --git a/gopsutil/systeminfo.go b/gopsutil/systeminfo.go index b50885241..13ada756e 100644 --- a/gopsutil/systeminfo.go +++ b/gopsutil/systeminfo.go @@ -139,9 +139,7 @@ func (s *systemInfo) collectPlatformInfo() error { // we have no way to know, let's try runtime s.cpuLogicalCores = runtime.NumCPU() } - if err != nil { - return err - } + return nil } return nil } diff --git a/holder.go b/holder.go index d751c90b2..40b21e538 100644 --- a/holder.go +++ b/holder.go @@ -594,9 +594,10 @@ func (h *Holder) loadNodeID() (string, error) { } nodeIDBytes, err := ioutil.ReadFile(idPath) - if err == nil { - nodeID = strings.TrimSpace(string(nodeIDBytes)) - } else if os.IsNotExist(err) { + // apparently it's safe to call IsNotExist on something that might + // be nil: + // https://github.com/golang/go/issues/31065 + if os.IsNotExist(err) { nodeID = uuid.NewV4().String() err = ioutil.WriteFile(idPath, []byte(nodeID), 0600) if err != nil { @@ -605,6 +606,7 @@ func (h *Holder) loadNodeID() (string, error) { } else if err != nil { return "", errors.Wrap(err, "reading file") } + nodeID = strings.TrimSpace(string(nodeIDBytes)) return nodeID, nil } diff --git a/holder_internal_test.go b/holder_internal_test.go index b13df7809..0c96a81ea 100644 --- a/holder_internal_test.go +++ b/holder_internal_test.go @@ -112,8 +112,10 @@ func TestHolder_Optn(t *testing.T) { } else if err := os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard"), 0000); err != nil { t.Fatal(err) } - defer os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard"), 0777) - + defer func() { + // we don't care about a failure here + _ = os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard"), 0755) + }() if err := h.Reopen(); err == nil || !strings.Contains(err.Error(), "permission denied") { t.Fatalf("unexpected error: %s", err) } @@ -136,7 +138,10 @@ func TestHolder_Optn(t *testing.T) { } else if err := os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments"), 0000); err != nil { t.Fatal(err) } - defer os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments"), 0777) + defer func() { + // we don't care about a failure here + _ = os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments"), 0755) + }() if err := h.Reopen(); err == nil || !strings.Contains(err.Error(), "permission denied") { t.Fatalf("unexpected error: %s", err) @@ -165,8 +170,9 @@ func TestHolder_Optn(t *testing.T) { } else if err := os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0.cache"), 0000); err != nil { t.Fatal(err) } - defer os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0.cache"), 0666) - + defer func() { + _ = os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0.cache"), 0644) + }() if err := h.Reopen(); err == nil || !strings.Contains(err.Error(), "permission denied") { t.Fatalf("unexpected error: %s", err) } @@ -209,8 +215,14 @@ func TestHolderCleaner_CleanHolder(t *testing.T) { hldr0.SetBit("y", "z", 10, (2*ShardWidth)+7) // Set highest shard. - hldr0.Field("i", "f").AddRemoteAvailableShards(roaring.NewBitmap(0, 1)) - hldr0.Field("y", "z").AddRemoteAvailableShards(roaring.NewBitmap(0, 1, 2)) + err := hldr0.Field("i", "f").AddRemoteAvailableShards(roaring.NewBitmap(0, 1)) + if err != nil { + t.Fatalf("adding remote shards: %v", err) + } + err = hldr0.Field("y", "z").AddRemoteAvailableShards(roaring.NewBitmap(0, 1, 2)) + if err != nil { + t.Fatalf("adding remote shards: %v", err) + } // Keep replication the same and ensure we get the expected results. cluster.ReplicaN = 2 @@ -292,8 +304,20 @@ func TestHolderCleaner_CleanHolder(t *testing.T) { func TestHolderCleaner_Reopen(t *testing.T) { h := NewHolder() h.Path = "path" - h.Open() - h.Close() - h.Open() - h.Close() + err := h.Open() + if err != nil { + t.Fatalf("couldn't open holder: %v", err) + } + err = h.Close() + if err != nil { + t.Fatalf("couldn't close holder: %v", err) + } + err = h.Open() + if err != nil { + t.Fatalf("couldn't open holder: %v", err) + } + err = h.Close() + if err != nil { + t.Fatalf("couldn't close holder: %v", err) + } } diff --git a/holder_test.go b/holder_test.go index 1bfe4615a..525ca1dc7 100644 --- a/holder_test.go +++ b/holder_test.go @@ -67,7 +67,9 @@ func TestHolder_Open(t *testing.T) { } else if err := os.Chmod(h.IndexPath("test"), 0000); err != nil { t.Fatal(err) } - defer os.Chmod(h.IndexPath("test"), 0777) + defer func() { + _ = os.Chmod(h.IndexPath("test"), 0755) + }() if err := h.Reopen(); err == nil || !strings.Contains(err.Error(), "permission denied") { t.Fatalf("unexpected error: %s", err) @@ -106,8 +108,9 @@ func TestHolder_Open(t *testing.T) { } else if err := os.Chmod(filepath.Join(h.Path, "foo", "bar"), 0000); err != nil { t.Fatal(err) } - defer os.Chmod(filepath.Join(h.Path, "foo", "bar"), 0777) - + defer func() { + _ = os.Chmod(filepath.Join(h.Path, "foo", "bar"), 0755) + }() if err := h.Reopen(); err == nil || !strings.Contains(err.Error(), "permission denied") { t.Fatalf("unexpected error: %s", err) } @@ -167,8 +170,9 @@ func TestHolder_Open(t *testing.T) { } else if err := os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0"), 0000); err != nil { t.Fatal(err) } - defer os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0"), 0666) - + defer func() { + _ = os.Chmod(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0"), 0644) + }() if err := h.Reopen(); err == nil || !strings.Contains(err.Error(), "permission denied") { t.Fatalf("unexpected error: %s", err) } diff --git a/http/client.go b/http/client.go index 92f6269fe..f8ad7cb37 100644 --- a/http/client.go +++ b/http/client.go @@ -629,7 +629,10 @@ func (c *InternalClient) ImportRoaring(ctx context.Context, uri *pilosa.URI, ind dec := json.NewDecoder(resp.Body) rbody := &pilosa.ImportResponse{} - dec.Decode(rbody) + err = dec.Decode(rbody) + if err != nil { + return errors.Wrap(err, "decoding response body") + } if rbody.Err != "" { return errors.Wrap(errors.New(rbody.Err), "importing roaring") } diff --git a/http/client_test.go b/http/client_test.go index 2b65f82d4..0bda138ca 100644 --- a/http/client_test.go +++ b/http/client_test.go @@ -116,9 +116,18 @@ func TestClient_MultiNode(t *testing.T) { // Rebuild the RankCache. // We have to do this to avoid the 10-second cache invalidation delay // built into cache.Invalidate() - c[0].RecalculateCaches() - c[1].RecalculateCaches() - c[2].RecalculateCaches() + err = c[0].RecalculateCaches() + if err != nil { + t.Fatalf("recalculating cache: %v", err) + } + err = c[1].RecalculateCaches() + if err != nil { + t.Fatalf("recalculating cache: %v", err) + } + err = c[2].RecalculateCaches() + if err != nil { + t.Fatalf("recalculating cache: %v", err) + } // Connect to each node to compare results. client := make([]*Client, 3) diff --git a/http/handler.go b/http/handler.go index 5b79deef6..bbb6e28a2 100644 --- a/http/handler.go +++ b/http/handler.go @@ -320,6 +320,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // successResponse is a general success/error struct for http responses. type successResponse struct { + h *Handler Success bool `json:"success"` Error *Error `json:"error,omitempty"` } @@ -367,8 +368,18 @@ func (r *successResponse) write(w http.ResponseWriter, err error) { // Write the response. if statusCode == 0 { - w.Write(msg) - w.Write([]byte("\n")) + _, err := w.Write(msg) + if err != nil { + r.h.logger.Printf("error writing response: %v", err) + http.Error(w, string(msg), http.StatusInternalServerError) + return + } + _, err = w.Write([]byte("\n")) + if err != nil { + r.h.logger.Printf("error writing newline after response: %v", err) + http.Error(w, string(msg), http.StatusInternalServerError) + return + } } else { http.Error(w, string(msg), statusCode) } @@ -449,7 +460,10 @@ func (h *Handler) handlePostQuery(w http.ResponseWriter, r *http.Request) { req, err := h.readQueryRequest(r) if err != nil { w.WriteHeader(http.StatusBadRequest) - h.writeQueryResponse(w, r, &pilosa.QueryResponse{Err: err}) + e := h.writeQueryResponse(w, r, &pilosa.QueryResponse{Err: err}) + if e != nil { + h.logger.Printf("write query response error: %v (while trying to write another error: %v)", e, err) + } return } // TODO: Remove @@ -463,7 +477,10 @@ func (h *Handler) handlePostQuery(w http.ResponseWriter, r *http.Request) { default: w.WriteHeader(http.StatusBadRequest) } - h.writeQueryResponse(w, r, &pilosa.QueryResponse{Err: err}) + e := h.writeQueryResponse(w, r, &pilosa.QueryResponse{Err: err}) + if e != nil { + h.logger.Printf("write query response error: %v (while trying to write another error: %v)", e, err) + } return } @@ -612,7 +629,7 @@ func (h *Handler) handleDeleteIndex(w http.ResponseWriter, r *http.Request) { indexName := mux.Vars(r)["index"] - resp := successResponse{} + resp := successResponse{h: h} err := h.api.DeleteIndex(r.Context(), indexName) resp.write(w, err) } @@ -625,7 +642,7 @@ func (h *Handler) handlePostIndex(w http.ResponseWriter, r *http.Request) { } indexName := mux.Vars(r)["index"] - resp := successResponse{} + resp := successResponse{h: h} // Decode request. req := postIndexRequest{ @@ -694,7 +711,7 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { indexName := mux.Vars(r)["index"] fieldName := mux.Vars(r)["field"] - resp := successResponse{} + resp := successResponse{h: h} // Decode request. var req postFieldRequest @@ -847,7 +864,7 @@ func (h *Handler) handleDeleteField(w http.ResponseWriter, r *http.Request) { indexName := mux.Vars(r)["index"] fieldName := mux.Vars(r)["field"] - resp := successResponse{} + resp := successResponse{h: h} err := h.api.DeleteField(r.Context(), indexName, fieldName) resp.write(w, err) } @@ -863,7 +880,7 @@ func (h *Handler) handleDeleteRemoteAvailableShard(w http.ResponseWriter, r *htt fieldName := mux.Vars(r)["field"] shardID, _ := strconv.ParseUint(mux.Vars(r)["shardID"], 10, 64) - resp := successResponse{} + resp := successResponse{h: h} err := h.api.DeleteAvailableShard(r.Context(), indexName, fieldName, shardID) resp.write(w, err) } @@ -1080,7 +1097,10 @@ func (h *Handler) handlePostImport(w http.ResponseWriter, r *http.Request) { } // Write response. - w.Write(buf) + _, err = w.Write(buf) + if err != nil { + h.logger.Printf("writing import response: %v", err) + } } // handleGetExport handles /export requests. @@ -1179,7 +1199,10 @@ func (h *Handler) handleGetFragmentBlockData(w http.ResponseWriter, r *http.Requ // Write response. w.Header().Set("Content-Type", "application/protobuf") w.Header().Set("Content-Length", strconv.Itoa(len(buf))) - w.Write(buf) + _, err = w.Write(buf) + if err != nil { + h.logger.Printf("writing fragment/block/data response: %v", err) + } } // handleGetFragmentBlocks handles GET /internal/fragment/blocks requests. diff --git a/logger/logger.go b/logger/logger.go index ed5a2dc26..9e895d482 100644 --- a/logger/logger.go +++ b/logger/logger.go @@ -15,6 +15,7 @@ package logger import ( + "fmt" "io" "log" ) @@ -82,3 +83,25 @@ func (vb *verboseLogger) Debugf(format string, v ...interface{}) { func (vb *verboseLogger) Logger() *log.Logger { return vb.logger } + +// CaptureLogger is a logger that stores all the print and debug messages +// it sees, useful for testing. +type CaptureLogger struct { + Prints []string + Debugs []string +} + +// NewCaptureLogger yields a CaptureLogger. +func NewCaptureLogger() *CaptureLogger { + return &CaptureLogger{} +} + +// Printf formats a message and appends it to Prints. +func (cl *CaptureLogger) Printf(format string, v ...interface{}) { + cl.Prints = append(cl.Prints, fmt.Sprintf(format, v...)) +} + +// Debugf formats a message and appends it to Debugs. +func (cl *CaptureLogger) Debugf(format string, v ...interface{}) { + cl.Debugs = append(cl.Debugs, fmt.Sprintf(format, v...)) +} diff --git a/lru/lru.go b/lru/lru.go index ba0121a9a..7f2e6dc22 100644 --- a/lru/lru.go +++ b/lru/lru.go @@ -83,7 +83,7 @@ func (c *Cache) Get(key Key) (value interface{}, ok bool) { } // remove removes the provided key from the cache. -func (c *Cache) remove(key Key) { // nolint: staticcheck +func (c *Cache) remove(key Key) { // nolint: staticcheck,unused if c.cache == nil { return } @@ -121,7 +121,7 @@ func (c *Cache) Len() int { } // clear purges all stored items from the cache. -func (c *Cache) clear() { // nolint: staticcheck +func (c *Cache) clear() { // nolint: staticcheck,unused if c.OnEvicted != nil { for _, e := range c.cache { kv := e.Value.(*entry) diff --git a/pql/pql.peg.go b/pql/pql.peg.go index acec51d19..f1ae962fc 100644 --- a/pql/pql.peg.go +++ b/pql/pql.peg.go @@ -15,7 +15,7 @@ const endSymbol rune = 1114112 type pegRule uint8 const ( - ruleUnknown pegRule = iota + ruleUnknown pegRule = iota // nolint:varcheck,deadcode,unused ruleCalls ruleCall ruleallargs diff --git a/roaring/btree.go b/roaring/btree.go index 76427059f..192993c3b 100644 --- a/roaring/btree.go +++ b/roaring/btree.go @@ -870,8 +870,10 @@ func (e *enumerator) Next() (k uint64, v *Container, err error) { i := e.q.d[e.i] k, v = i.k, i.v e.k, e.hit = k, true - e.next() - return k, v, nil + // Any error returned would be stashed in e.err, and would come up + // on the next call. + _ = e.next() + return k, v, err } func (e *enumerator) next() error { @@ -925,7 +927,9 @@ func (e *enumerator) Prev() (k uint64, v *Container, err error) { i := e.q.d[e.i] k, v = i.k, i.v e.k, e.hit = k, true - e.prev() + // Any error returned would be stashed in e.err, and would come up + // on the next call. + _ = e.prev() return k, v, err } diff --git a/roaring/containers_btree.go b/roaring/containers_btree.go index 1d08f598c..5934a1244 100644 --- a/roaring/containers_btree.go +++ b/roaring/containers_btree.go @@ -35,7 +35,8 @@ func NewBTreeBitmap(a ...uint64) *Bitmap { b := &Bitmap{ Containers: newBTreeContainers(), } - b.Add(a...) + // TODO: We have no way to report this. + _, _ = b.Add(a...) return b } diff --git a/roaring/roaring.go b/roaring/roaring.go index a8ccd81e2..78b5f4ca8 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -137,7 +137,10 @@ func NewBitmap(a ...uint64) *Bitmap { b := &Bitmap{ Containers: newSliceContainers(), } - b.AddN(a...) + // TODO: We have no way to report this. We aren't in a server context + // so we haven't got a logger, nothing is checking for nil returns + // from this... + _, _ = b.AddN(a...) return b } @@ -3695,8 +3698,8 @@ func (op *op) WriteTo(w io.Writer) (n int64, err error) { // Add checksum at the end. h := fnv.New32a() - h.Write(buf[0:9]) - h.Write(buf[13:]) + _, _ = h.Write(buf[0:9]) + _, _ = h.Write(buf[13:]) binary.LittleEndian.PutUint32(buf[9:13], h.Sum32()) // Write to writer. @@ -3719,13 +3722,13 @@ func (op *op) UnmarshalBinary(data []byte) error { // Verify checksum. h := fnv.New32a() - h.Write(data[0:9]) + _, _ = h.Write(data[0:9]) if op.typ > 1 { if len(data) < int(13+op.value*8) { return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data)) } - h.Write(data[13 : 13+op.value*8]) + _, _ = h.Write(data[13 : 13+op.value*8]) op.values = make([]uint64, op.value) for i := uint64(0); i < op.value; i++ { start := 13 + i*8 diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index e88356a37..8c3e7a930 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -2127,10 +2127,14 @@ func TestIteratorBitmap(t *testing.T) { // but won't update to RLE until Optimize() is called b := NewFileBitmap() for i := uint64(61000); i < 71000; i++ { - b.Add(i) + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } } for i := uint64(75000); i < 75100; i++ { - b.Add(i) + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } } if !b.Containers.Get(0).isBitmap() { t.Fatalf("wrong container type") @@ -2393,7 +2397,9 @@ func TestRunBinSearch(t *testing.T) { } func TestBitmap_RemoveEmptyContainers(t *testing.T) { bm1 := NewFileBitmap(1<<16, 2<<16, 3<<16) - bm1.Remove(2 << 16) + if _, err := bm1.Remove(2 << 16); err != nil { + t.Fatalf("removing a bit: %v", err) + } if bm1.countEmptyContainers() != 1 { t.Fatalf("Should be 1 empty container ") } @@ -2406,13 +2412,17 @@ func TestBitmap_RemoveEmptyContainers(t *testing.T) { func TestBitmap_BitmapWriteToWithEmpty(t *testing.T) { bm1 := NewFileBitmap(1<<16, 2<<16, 3<<16) - bm1.Remove(2 << 16) + if _, err := bm1.Remove(2 << 16); err != nil { + t.Fatalf("removing a bit: %v", err) + } var buf bytes.Buffer if _, err := bm1.WriteTo(&buf); err != nil { t.Fatalf("Failure to write to bitmap buffer. ") } bm0 := NewFileBitmap() - bm0.UnmarshalBinary(buf.Bytes()) + if err := bm0.UnmarshalBinary(buf.Bytes()); err != nil { + t.Fatalf("unmarshalling: %v", err) + } if bm0.countEmptyContainers() != 0 { t.Fatalf("Should be no empty containers ") } @@ -2559,7 +2569,9 @@ func TestIntersectArrayBitmap(t *testing.T) { func TestBitmapClone(t *testing.T) { b := NewFileBitmap() for i := uint64(61000); i < 71000; i++ { - b.Add(i) + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } } c := b.Clone() if err := bitmapsEqual(b, c); err != nil { @@ -3770,24 +3782,45 @@ func TestBitmapAny(t *testing.T) { if bm.Any() { t.Error("empty bitmap should have Any()==false") } - bm.Add(1) + _, err := bm.Add(1) + if err != nil { + t.Errorf("couldn't add a bit: %v", err) + } if !bm.Any() { t.Error("bitmap with 1 bit should have Any()==true") } - bm.Add(100000) + _, err = bm.Add(100000) + if err != nil { + t.Errorf("couldn't add a bit: %v", err) + } if !bm.Any() { t.Error("bitmap with 2 bits should have Any()==true") } - bm.Remove(1) + changed, err := bm.Remove(1) + if err != nil { + t.Errorf("couldn't remove a bit: %v", err) + } + if changed != true { + t.Error("removing a set bit should have been a change") + } if !bm.Any() { t.Error("bitmap with 1 bit left after removing 1 should have Any()==true") } - bm.Add(1) + _, err = bm.Add(1) + if err != nil { + t.Errorf("couldn't remove a bit: %v", err) + } + if changed != true { + t.Error("re-addintg a previously set bit should have been a change") + } bm = bm.Difference(NewBTreeBitmap(1)) if !bm.Any() { t.Error("bitmap with 1 bit left after differencing 1 should have Any()==true") } - bm.Remove(100000) + _, err = bm.Remove(100000) + if err != nil { + t.Errorf("couldn't remove a bit: %v", err) + } if bm.Any() { t.Error("shouldn't be any left") } diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index ebf88a212..b4f792629 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -164,11 +164,15 @@ func TestCheckBitmap(t *testing.T) { x := 0 for i := uint64(61000); i < 71000; i++ { x++ - b.Add(i) + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } } for i := uint64(75000); i < 75100; i++ { x++ - b.Add(i) + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } } err := b.Check() if err != nil { @@ -198,7 +202,7 @@ func TestCheckFullRun(t *testing.T) { if i%16384 == 0 { b.Optimize() // convert to runs } - b.Add(i) + _, _ = b.Add(i) } err := b.Check() if err != nil { @@ -238,7 +242,13 @@ func TestBitmap_Contains_Empty(t *testing.T) { // Ensure an empty bitmap does nothing when removing an element. func TestBitmap_Remove_Empty(t *testing.T) { - roaring.NewFileBitmap().Remove(1000) + changed, err := roaring.NewFileBitmap().Remove(1000) + if err != nil { + t.Fatalf("got an error removing a bit from an empty bitmap: %v", err) + } + if changed != false { + t.Fatalf("change reported removing a bit from an empty bitmap") + } } // Ensure a bitmap can return a slice of values. @@ -289,7 +299,9 @@ func TestBitmap_ForEachRange(t *testing.T) { func TestBitmap_Max(t *testing.T) { bm := roaring.NewFileBitmap() for i := uint64(1000); i <= 100000; i++ { - bm.Add(i) + if _, err := bm.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } if v := bm.Max(); v != i { t.Fatalf("max: got=%d; want=%d", v, i) @@ -310,7 +322,9 @@ func TestBitmap_BitmapCountRangeEdgeCase(t *testing.T) { } else { start += 2 } - bm0.Add(start) + if _, err := bm0.Add(start); err != nil { + t.Fatalf("adding bit: %v", err) + } } a := bm0.Count() r := bm0.CountRange(s, e) @@ -323,9 +337,13 @@ func TestBitmap_BitmapCountRangeEdgeCase(t *testing.T) { func TestBitmap_BitmapCountRange(t *testing.T) { bm0 := roaring.NewFileBitmap(0, 2683177) for i := uint64(628); i < 2683301; i++ { - bm0.Add(i) + if _, err := bm0.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } + } + if _, err := bm0.Add(2683307); err != nil { + t.Fatalf("adding bits: %v", err) } - bm0.Add(2683307) if n := bm0.CountRange(1, 2683311); n != 2682674 { t.Fatalf("unexpected n: %d", n) } @@ -389,7 +407,9 @@ func TestBitmap_Intersection(t *testing.T) { bm0 := roaring.NewFileBitmap(0, 2683177) bm1 := roaring.NewFileBitmap() for i := uint64(628); i < 2683301; i++ { - bm1.Add(i) + if _, err := bm1.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } } result := bm0.Intersect(bm1) @@ -403,9 +423,13 @@ func TestBitmap_Union1(t *testing.T) { bm0 := roaring.NewFileBitmap(0, 2683177) bm1 := roaring.NewFileBitmap() for i := uint64(628); i < 2683301; i++ { - bm1.Add(i) + if _, err := bm1.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } + } + if _, err := bm1.Add(4000000); err != nil { + t.Fatalf("adding bits: %v", err) } - bm1.Add(4000000) result := bm0.Union(bm1) if n := result.Count(); n != 2682675 { @@ -429,9 +453,13 @@ func TestBitmap_UnionInPlace1(t *testing.T) { result = roaring.NewBitmap() ) for i := uint64(628); i < 2683301; i++ { - bm1.Add(i) + if _, err := bm1.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } + } + if _, err := bm1.Add(4000000); err != nil { + t.Fatalf("adding bits: %v", err) } - bm1.Add(4000000) result.UnionInPlace(bm0, bm1) if n := result.Count(); n != 2682675 { @@ -504,7 +532,9 @@ func TestBitmap_UnionInPlaceProp(t *testing.T) { // size of a container to ensure we generate a maxRange container. for x := start; x < (start + 2*(0xffff+1)); x++ { set[uint64(x)] = struct{}{} - bitmap.Add(uint64(x)) + if _, err := bitmap.Add(uint64(x)); err != nil { + t.Fatalf("adding bits: %v", err) + } } } @@ -513,7 +543,9 @@ func TestBitmap_UnionInPlaceProp(t *testing.T) { for x := 0; x < numIntsPerBatch; x++ { num := uint64(rng.Intn(maxUint64Val)) set[num] = struct{}{} - bitmap.Add(num) + if _, err := bitmap.Add(num); err != nil { + t.Fatalf("adding bits: %v", err) + } } sets = append(sets, set) @@ -588,12 +620,16 @@ func TestBitmap_IntersectArrayArray(t *testing.T) { func TestBitmap_IntersectBitmapBitmap(t *testing.T) { bm0 := roaring.NewFileBitmap() for i := uint64(0); i < 65536; i += 2 { - bm0.Add(i) + if _, err := bm0.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } } bm1 := roaring.NewFileBitmap() for i := uint64(0); i < 65536; i += 3 { - bm1.Add(i) + if _, err := bm1.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } } result := bm0.Intersect(bm1) @@ -620,7 +656,9 @@ func TestBitmap_IntersectRunRun(t *testing.T) { offset := (runLen / 2) + spaceLen for i := uint64(0); i < (65536 - runLen - offset); i += (runLen + spaceLen) { for j := uint64(0); j < runLen; j++ { - bm2.Add(offset + i + j) + if _, err := bm2.Add(offset + i + j); err != nil { + t.Fatalf("adding bits: %v", err) + } } } bm2.Optimize() // convert to runs @@ -629,7 +667,9 @@ func TestBitmap_IntersectRunRun(t *testing.T) { spaceLen = uint64(1) for i := uint64(0); i < (65536 - runLen); i += (runLen + spaceLen) { for j := uint64(0); j < runLen; j++ { - bm3.Add(i + j) + if _, err := bm3.Add(i + j); err != nil { + t.Fatalf("adding bits: %v", err) + } } } bm3.Optimize() // convert to runs @@ -643,7 +683,7 @@ func TestBitmap_Difference(t *testing.T) { bm0 := roaring.NewFileBitmap(0, 2683177) bm1 := roaring.NewFileBitmap() for i := uint64(628); i < 2683301; i++ { - bm1.Add(i) + _, _ = bm1.Add(i) } result := bm0.Difference(bm1) if n := result.Count(); n != 1 { @@ -771,7 +811,7 @@ func TestBitmap_Xor_ArrayBitmap(t *testing.T) { bm0 := roaring.NewFileBitmap(1, 70, 200, 4097, 4098) bm1 := roaring.NewFileBitmap() for i := uint64(0); i < 10000; i += 2 { - bm1.Add(i) + _, _ = bm1.Add(i) } result := bm0.Xor(bm1) @@ -802,11 +842,11 @@ func TestBitmap_Xor_BitmapBitmap(t *testing.T) { bm1 := roaring.NewFileBitmap() for i := uint64(0); i < 10000; i += 2 { - bm1.Add(i) + _, _ = bm1.Add(i) } for i := uint64(1); i < 10000; i += 2 { - bm0.Add(i) + _, _ = bm0.Add(i) } result := bm0.Xor(bm1) @@ -847,7 +887,9 @@ func TestBitmap_Flip_Bitmap(t *testing.T) { bm := roaring.NewFileBitmap() size := uint64(10000) for i := uint64(0); i < size; i += 2 { - bm.Add(i) + if _, err := bm.Add(i); err != nil { + t.Fatalf("adding bits: %v", err) + } } results := bm.Flip(0, size-1) if n := results.Count(); n != size/2 { @@ -921,7 +963,7 @@ func TestBitmap_IntersectionCount_RunRun(t *testing.T) { func TestBitmap_IntersectionCount_BitmapRun(t *testing.T) { bm0 := roaring.NewFileBitmap() for i := uint64(3); i <= 1000006; i += 2 { - bm0.Add(i) + _, _ = bm0.Add(i) } bm1 := roaring.NewFileBitmap(0, 1, 2, 3, 4, 5, 1000000, 1000002, 1000003, 1000004, 1000005, 1000006) bm1.Optimize() // convert to runs @@ -938,7 +980,7 @@ func TestBitmap_IntersectionCount_ArrayBitmap(t *testing.T) { bm0 := roaring.NewFileBitmap(1, 70, 200, 4097, 4098) bm1 := roaring.NewFileBitmap() for i := uint64(0); i <= 10000; i += 2 { - bm1.Add(i) + _, _ = bm1.Add(i) } if n := bm0.IntersectionCount(bm1); n != 3 { @@ -953,15 +995,15 @@ func TestBitmap_IntersectionCount_BitmapBitmap(t *testing.T) { bm0 := roaring.NewFileBitmap() bm1 := roaring.NewFileBitmap() for i := uint64(0); i <= 10000; i += 2 { - bm0.Add(i) - bm1.Add(i + 1) + _, _ = bm0.Add(i) + _, _ = bm1.Add(i + 1) } - bm0.Add(1000) - bm1.Add(1000) + _, _ = bm0.Add(1000) + _, _ = bm1.Add(1000) - bm0.Add(2000) - bm1.Add(2000) + _, _ = bm0.Add(2000) + _, _ = bm1.Add(2000) if n := bm0.IntersectionCount(bm1); n != 2 { t.Fatalf("unexpected n: %d", n) @@ -1005,7 +1047,7 @@ func TestBitmap_Quick_LargeValue(t *testing.T) { testBitmapQuick(t, 10000, 0, ma // Ensure a bitmap can perform basic operations on randomly generated values. func testBitmapQuick(t *testing.T, n int, min, max uint64) { - quick.Check(func(a []uint64) bool { + err := quick.Check(func(a []uint64) bool { bm := roaring.NewFileBitmap() m := make(map[uint64]struct{}) @@ -1067,6 +1109,9 @@ func testBitmapQuick(t *testing.T, n int, min, max uint64) { values[0] = reflect.ValueOf(GenerateUint64Slice(n, min, max, false, rand)) }, }) + if err != nil { + t.Fatalf("quick check failed: %v", err) + } } func TestBitmap_Marshal_Quick_Array1(t *testing.T) { testBitmapMarshalQuick(t, 1000, 1000, 2000, false) } @@ -1091,7 +1136,7 @@ func testBitmapMarshalQuick(t *testing.T, n int, min, max uint64, sorted bool) { t.Skip("short") } - quick.Check(func(a0, a1 []uint64) bool { + err := quick.Check(func(a0, a1 []uint64) bool { // Create bitmap with initial values set. bm := roaring.NewFileBitmap(a0...) @@ -1145,6 +1190,9 @@ func testBitmapMarshalQuick(t *testing.T, n int, min, max uint64, sorted bool) { values[1] = reflect.ValueOf(GenerateUint64Slice(100, min, max, sorted, rand)) }, }) + if err != nil { + t.Fatalf("quick check failed: %v", err) + } } // Ensure iterator can iterate over all the values on the bitmap. @@ -1167,13 +1215,13 @@ func TestIterator(t *testing.T) { t.Run("run", func(t *testing.T) { bm1 := roaring.NewFileBitmap() for i := uint64(0); i < 11; i += 1 { - bm1.Add(i) + _, _ = bm1.Add(i) } bm1.Optimize() bm2 := roaring.NewFileBitmap() for i := uint64(0); i < 12; i += 1 { - bm2.Add(i) + _, _ = bm2.Add(i) } bm2.Optimize() @@ -1203,23 +1251,24 @@ func TestIterator(t *testing.T) { // testBM creates a bitmap with 3 containers: array, bitmap, and run. func testBM() *roaring.Bitmap { - + // We should possibly be testing the adds for errors, but we + // don't have a clean way to return an error, so we don't right now. bm := roaring.NewFileBitmap() //the array for i := uint64(0); i < 1024; i += 4 { - bm.Add((1 << 16) + i) + _, _ = bm.Add((1 << 16) + i) } //the bitmap for i := uint64(0); i < 16384; i += 2 { - bm.Add((2 << 16) + i) + _, _ = bm.Add((2 << 16) + i) } //small run for i := uint64(0); i < 1024; i += 1 { - bm.Add((3 << 16) + i) + _, _ = bm.Add((3 << 16) + i) } //large run for i := uint64(0); i < 65535; i += 1 { - bm.Add((4 << 16) + i) + _, _ = bm.Add((4 << 16) + i) } bm.Optimize() //count 75007 @@ -1275,9 +1324,13 @@ func isAllType(b *roaring.Bitmap, typ string) bool { return true } +// getBenchData yields some sample data func getBenchData(tb testing.TB) *benchmarkSampleData { data := &sampleData if data.a1 == nil { + // throughout this, we ignore any errors from bitmap adds, + // because errors in those should result in the Optimize + // pass producing the wrong values, so we can just check there. const max = (1 << 24) / 64 // Build bitmap with array container. @@ -1285,28 +1338,28 @@ func getBenchData(tb testing.TB) *benchmarkSampleData { data.a2 = roaring.NewFileBitmap() // two lists of different lengths for i, n := 0, roaring.ArrayMaxSize/3; i < n; i++ { - data.a1.Add(uint64(rand.Intn(max))) - data.a2.Add(uint64(rand.Intn(max))) + _, _ = data.a1.Add(uint64(rand.Intn(max))) + _, _ = data.a2.Add(uint64(rand.Intn(max))) } for i, n := 0, roaring.ArrayMaxSize/3; i < n; i++ { - data.a1.Add(uint64(rand.Intn(max))) + _, _ = data.a1.Add(uint64(rand.Intn(max))) } // Build bitmap with bitmap container. data.b = roaring.NewFileBitmap() for i, n := 0, MaxContainerVal/3; i < n; i++ { - data.b.Add(uint64(i * 3)) + _, _ = data.b.Add(uint64(i * 3)) } // build bitmap with run container data.r1 = roaring.NewFileBitmap() for i, n := 0, MaxContainerVal; i < n; i++ { - data.r1.Add(uint64(i)) + _, _ = data.r1.Add(uint64(i)) } // build bitmap with multiple runs data.r2 = roaring.NewFileBitmap() for i, n := 0, MaxContainerVal; i < n; i++ { - data.r2.Add(uint64(i)) + _, _ = data.r2.Add(uint64(i)) // break the runs up, this should produce 16 runs, which // is small enough to make RLE tempting if i&0xfff == 0xfff { @@ -1467,7 +1520,7 @@ func BenchmarkContainerLinear(b *testing.B) { bm := bmMaker() for row := uint64(1); row < NumRows; row++ { for col := uint64(1); col < NumColums; col++ { - bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) } } } @@ -1483,7 +1536,7 @@ func BenchmarkContainerReverse(b *testing.B) { bm := bmMaker() for row := NumRows - 1; row >= 1; row-- { for col := NumColums - 1; col >= 1; col-- { - bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) } } } @@ -1498,7 +1551,7 @@ func BenchmarkContainerColumn(b *testing.B) { bm := bmMaker() for col := uint64(1); col < NumColums; col++ { for row := uint64(1); row < NumRows; row++ { - bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) } } } @@ -1514,8 +1567,8 @@ func BenchmarkContainerOutsideIn(b *testing.B) { bm := bmMaker() for col := uint64(1); col < NumColums; col++ { for row := uint64(1); row < middle; row++ { - bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) - bm.Add((NumRows-row)*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add(row*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add((NumRows-row)*pilosa.ShardWidth + (col * MaxContainerVal)) } } } @@ -1532,8 +1585,8 @@ func BenchmarkContainerInsideOut(b *testing.B) { bm := bmMaker() for col := uint64(1); col < NumColums; col++ { for row := uint64(1); row <= middle; row++ { - bm.Add((middle+row)*pilosa.ShardWidth + (col * MaxContainerVal)) - bm.Add((middle-row)*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add((middle+row)*pilosa.ShardWidth + (col * MaxContainerVal)) + _, _ = bm.Add((middle-row)*pilosa.ShardWidth + (col * MaxContainerVal)) } } } @@ -1545,7 +1598,7 @@ func BenchmarkSliceAscending(b *testing.B) { for n := 0; n < b.N; n++ { bm := roaring.NewFileBitmap() for col := uint64(0); col < pilosa.ShardWidth; col++ { - bm.Add(col) + _, _ = bm.Add(col) } } } @@ -1554,9 +1607,9 @@ func BenchmarkSliceDescending(b *testing.B) { for n := 0; n < b.N; n++ { bm := roaring.NewFileBitmap() for col := uint64(pilosa.ShardWidth); col > uint64(0); col-- { - bm.Add(col) + _, _ = bm.Add(col) } - bm.Add(0) + _, _ = bm.Add(0) } } @@ -1565,14 +1618,14 @@ func BenchmarkSliceAscendingStriped(b *testing.B) { bm := roaring.NewFileBitmap() l := uint64(pilosa.ShardWidth / 8) for col := uint64(0); col < l; col++ { - bm.Add(l*0 + col) - bm.Add(l*1 + col) - bm.Add(l*2 + col) - bm.Add(l*3 + col) - bm.Add(l*4 + col) - bm.Add(l*5 + col) - bm.Add(l*6 + col) - bm.Add(l*7 + col) + _, _ = bm.Add(l*0 + col) + _, _ = bm.Add(l*1 + col) + _, _ = bm.Add(l*2 + col) + _, _ = bm.Add(l*3 + col) + _, _ = bm.Add(l*4 + col) + _, _ = bm.Add(l*5 + col) + _, _ = bm.Add(l*6 + col) + _, _ = bm.Add(l*7 + col) } } } @@ -1582,14 +1635,14 @@ func BenchmarkSliceDescendingStriped(b *testing.B) { bm := roaring.NewFileBitmap() l := uint64(pilosa.ShardWidth / 8) for col := uint64(l); col < l+1; col-- { - bm.Add(l*7 + col) - bm.Add(l*6 + col) - bm.Add(l*5 + col) - bm.Add(l*4 + col) - bm.Add(l*3 + col) - bm.Add(l*2 + col) - bm.Add(l*1 + col) - bm.Add(l*0 + col) + _, _ = bm.Add(l*7 + col) + _, _ = bm.Add(l*6 + col) + _, _ = bm.Add(l*5 + col) + _, _ = bm.Add(l*4 + col) + _, _ = bm.Add(l*3 + col) + _, _ = bm.Add(l*2 + col) + _, _ = bm.Add(l*1 + col) + _, _ = bm.Add(l*0 + col) } } } diff --git a/server.go b/server.go index 3e989e530..a4d05b144 100644 --- a/server.go +++ b/server.go @@ -559,7 +559,7 @@ func (s *Server) receiveMessage(m Message) error { return err } case *SetCoordinatorMessage: - s.cluster.setCoordinator(obj.New) + return s.cluster.setCoordinator(obj.New) case *UpdateCoordinatorMessage: s.cluster.updateCoordinator(obj.New) case *NodeStateMessage: @@ -705,7 +705,10 @@ func (s *Server) monitorDiagnostics() { s.diagnostics.Set("GoRoutines", runtime.NumGoroutine()) s.diagnostics.EnrichWithMemoryInfo() s.diagnostics.EnrichWithSchemaProperties() - s.diagnostics.CheckVersion() + err = s.diagnostics.CheckVersion() + if err != nil { + s.logger.Printf("can't check version: %v", err) + } err = s.diagnostics.Flush() if err != nil { s.logger.Printf("diagnostics error: %s", err) diff --git a/server/handler_test.go b/server/handler_test.go index ff9654933..082869081 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -846,7 +846,10 @@ func TestClusterTranslator(t *testing.T) { cluster := make(test.Cluster, 2) cluster[0] = test.NewCommandNode(true) cluster[0].Config.Gossip.Port = "0" - cluster[0].Start() + err := cluster[0].Start() + if err != nil { + t.Fatalf("starting cluster 1: %v", err) + } httpTranslateStore := http.NewTranslateStore(cluster[0].URL()) cluster[1] = test.NewCommandNode(false, server.OptCommandServerOptions( @@ -855,7 +858,10 @@ func TestClusterTranslator(t *testing.T) { ) cluster[1].Config.Gossip.Port = "0" cluster[1].Config.Gossip.Seeds = []string{cluster[0].GossipAddress()} - cluster[1].Start() + err = cluster[1].Start() + if err != nil { + t.Fatalf("starting cluster 1: %v", err) + } test.MustDo("POST", cluster[0].URL()+"/index/i0", "{\"options\": {\"keys\": true}}") test.MustDo("POST", cluster[0].URL()+"/index/i0/field/f0", "{\"options\": {\"keys\": true}}") diff --git a/server/server_test.go b/server/server_test.go index 1cc3bd87f..19b79c6ea 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -900,7 +900,7 @@ func TestClusterExhaustingConnections(t *testing.T) { return nil }) } - err := eg.Wait() + err = eg.Wait() if err != nil { t.Fatalf("setting lots of shards: %v", err) } @@ -929,7 +929,10 @@ func TestClusterExhaustingConnectionsImport(t *testing.T) { bm := roaring.NewBitmap() bm.DirectAdd(0) buf := &bytes.Buffer{} - bm.WriteTo(buf) + _, err := bm.WriteTo(buf) + if err != nil { + t.Fatalf("writing to buffer: %v", err) + } data := buf.Bytes() eg := errgroup.Group{} diff --git a/utils_internal_test.go b/utils_internal_test.go index 321f4846d..3396cb4e9 100644 --- a/utils_internal_test.go +++ b/utils_internal_test.go @@ -57,15 +57,15 @@ func NewTestCluster(n int) *cluster { // NewTestURI is a test URI creator that intentionally swallows errors. func NewTestURI(scheme, host string, port uint16) URI { uri := defaultURI() - uri.setScheme(scheme) - uri.setHost(host) + _ = uri.setScheme(scheme) + _ = uri.setHost(host) uri.SetPort(port) return *uri } func NewTestURIFromHostPort(host string, port uint16) URI { uri := defaultURI() - uri.setHost(host) + _ = uri.setHost(host) uri.SetPort(port) return *uri } diff --git a/view.go b/view.go index 5780e682c..b1e5dee78 100644 --- a/view.go +++ b/view.go @@ -173,7 +173,7 @@ func (v *view) availableShards() *roaring.Bitmap { b := roaring.NewBitmap() for shard := range v.fragments { - b.Add(shard) // ignore error, no writer attached + _, _ = b.Add(shard) // ignore error, no writer attached } return b } From d5907b2a2e86aafcaf70a5be794512cc23a23777 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 29 Mar 2019 17:29:21 -0500 Subject: [PATCH 07/19] lint fixes to cluster behavior in utils test This is more lint fixes, but it's less obvious to me what the right handling for errors is, or whether disregarding them is safe, so it's a separate commit. --- utils_internal_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/utils_internal_test.go b/utils_internal_test.go index 3396cb4e9..2f21fb04b 100644 --- a/utils_internal_test.go +++ b/utils_internal_test.go @@ -235,7 +235,9 @@ func (t *ClusterCluster) addCluster(i int, saveTopology bool) (*cluster, error) // add nodes if saveTopology { for _, n := range t.common.Nodes { - c.addNode(n) + if err := c.addNode(n); err != nil { + return nil, err + } } } @@ -314,7 +316,10 @@ func (b bcast) SendSync(m Message) error { // Apply the send message to all nodes (except the coordinator). for _, c := range b.t.Clusters { if c != b.c { - c.mergeClusterStatus(obj) + err := c.mergeClusterStatus(obj) + if err != nil { + return err + } } } b.t.mu.RLock() @@ -348,7 +353,9 @@ func (b bcast) SendTo(to *Node, m Message) error { } case *ResizeInstructionComplete: coord := b.t.clusterByID(to.ID) - go coord.markResizeInstructionComplete(obj) + // this used to be async, but that prevented us from checking + // its error status... + return coord.markResizeInstructionComplete(obj) case *ClusterStatus: // Apply the send message to the node. for _, c := range b.t.Clusters { From 2c6eb6689588d256407bef51cc0103023060edd8 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 1 Apr 2019 15:56:14 -0500 Subject: [PATCH 08/19] check for slightly fewer errors json.Decoder.Decode() can yield io.EOF which is not actually an error. This appears to have caused a number of indirect test failures by making ImportRoaring generally report failure. --- http/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/http/client.go b/http/client.go index f8ad7cb37..4f2f1c208 100644 --- a/http/client.go +++ b/http/client.go @@ -630,7 +630,8 @@ func (c *InternalClient) ImportRoaring(ctx context.Context, uri *pilosa.URI, ind dec := json.NewDecoder(resp.Body) rbody := &pilosa.ImportResponse{} err = dec.Decode(rbody) - if err != nil { + // Decode can return EOF when no error occurred. helpful! + if err != nil && err != io.EOF { return errors.Wrap(err, "decoding response body") } if rbody.Err != "" { From 79451bd53cc3be36d4c8e788465a4cf4642197d4 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 1 Apr 2019 19:05:47 -0500 Subject: [PATCH 09/19] undo accidental change to test case contents --- ctl/import_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ctl/import_test.go b/ctl/import_test.go index b20919203..01abf372c 100644 --- a/ctl/import_test.go +++ b/ctl/import_test.go @@ -313,7 +313,7 @@ func TestImportCommand_RunValueKeys(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = file.Write([]byte("foo1,bar2\nfoo3,bar4\nfoo5,bar6")) + _, err = file.Write([]byte("foo1,2\nfoo3,4\nfoo5,6")) if err != nil { t.Fatalf("writing to tempfile: %v", err) } @@ -355,7 +355,7 @@ func TestImportCommand_InvalidFile(t *testing.T) { if err != nil { t.Fatalf("creating tempfile: %v", err) } - _, err = file.Write([]byte("1,2\n3,4\n5,6")) + _, err = file.Write([]byte("a,2\n3,5\n5,6")) if err != nil { t.Fatalf("writing to tempfile: %v", err) } From c9cebe21bf2216d3e5f3163bfb039479a43b5b52 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 1 Apr 2019 19:34:34 -0500 Subject: [PATCH 10/19] unbreak holder node ID logic The attempt to fix up the logic broke returns from loadNodeID() in some cases, because it was overwriting the node ID generated in the IsNotExist case. --- holder.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/holder.go b/holder.go index 40b21e538..41ca16a9d 100644 --- a/holder.go +++ b/holder.go @@ -587,27 +587,23 @@ func (h *Holder) setFileLimit() { func (h *Holder) loadNodeID() (string, error) { idPath := path.Join(h.Path, ".id") - nodeID := "" h.Logger.Printf("load NodeID: %s", idPath) if err := os.MkdirAll(h.Path, 0777); err != nil { return "", errors.Wrap(err, "creating directory") } nodeIDBytes, err := ioutil.ReadFile(idPath) - // apparently it's safe to call IsNotExist on something that might - // be nil: - // https://github.com/golang/go/issues/31065 - if os.IsNotExist(err) { - nodeID = uuid.NewV4().String() - err = ioutil.WriteFile(idPath, []byte(nodeID), 0600) - if err != nil { - return "", errors.Wrap(err, "writing file") - } - } else if err != nil { + if err == nil { + return strings.TrimSpace(string(nodeIDBytes)), nil + } + if !os.IsNotExist(err) { return "", errors.Wrap(err, "reading file") } - nodeID = strings.TrimSpace(string(nodeIDBytes)) - + nodeID := uuid.NewV4().String() + err = ioutil.WriteFile(idPath, []byte(nodeID), 0600) + if err != nil { + return "", errors.Wrap(err, "writing file") + } return nodeID, nil } From 9fc8a5b352e5b03a6373857e264a24463e9b9b9d Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 1 Apr 2019 19:47:59 -0500 Subject: [PATCH 11/19] handle a specific error that might be an expected error I'm honestly not sure here. --- ctl/inspect_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ctl/inspect_test.go b/ctl/inspect_test.go index 265d77a12..69efe07fa 100644 --- a/ctl/inspect_test.go +++ b/ctl/inspect_test.go @@ -41,7 +41,7 @@ func TestInspectCommand_Run(t *testing.T) { file.Close() cm.Path = file.Name() err = cm.Run(context.Background()) - if err != nil { + if err != nil && err.Error() != "unmarshalling: reading roaring header: did not find expected serialCookie in header" { t.Fatalf("can't run command: %v", err) } From f15347064f81691a824840a65f43d04c83843e0b Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 1 Apr 2019 22:15:19 -0500 Subject: [PATCH 12/19] fix race in cluster state transition The anonymous goroutine, if it gets an error, can race with other changes. Make the values we intend to call it on parameters so it will work with those even if other things are happening. --- cluster.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cluster.go b/cluster.go index 024c426a1..4f865dc36 100644 --- a/cluster.go +++ b/cluster.go @@ -1892,12 +1892,12 @@ func (c *cluster) mergeClusterStatus(cs *ClusterStatus) error { for _, node := range officialNodes { if node.ID == c.Node.ID && node.State != c.Node.State { c.logger.Printf("mismatched state in mergeClusterStatus got %v have %v", node.State, c.Node.State) - go func() { - err := c.setNodeState(c.Node.State) + go func(fromState, toState string) { + err := c.setNodeState(toState) if err != nil { - c.logger.Printf("error setting node state from %v to %v: %v", node.State, c.Node.State, err) + c.logger.Printf("error setting node state from %v to %v: %v", fromState, toState, err) } - }() + }(node.State, c.Node.State) } if err := c.addNode(node); err != nil { return errors.Wrap(err, "adding node") From e61e62a695abe739e46ea4733d6e186461247c5f Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 5 Apr 2019 15:19:53 -0500 Subject: [PATCH 13/19] don't run gometalinter on CI anymore gometalinter is slow, golangci-lint is fast and checks a lot more things, let's just use that. We leave the old targets in the Makefile for now so we can use them for sanity-checking the results. --- .circleci/config.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index eb47ffcf8..415dd4411 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -24,12 +24,6 @@ jobs: - persist_to_workspace: root: . paths: "*" - linter: - <<: *defaults - steps: - - *fast-checkout - - run: make install-gometalinter - - run: make gometalinter check-license-headers: <<: *defaults steps: From babcf8c33122cd3fb5414a266a39a74ba6aea7ee Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 8 Apr 2019 11:05:02 -0500 Subject: [PATCH 14/19] continue having a linter target --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 415dd4411..d55e8704b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -29,7 +29,7 @@ jobs: steps: - *fast-checkout - run: make check-license-headers - golangci-lint: + linter: <<: *defaults steps: - *fast-checkout From 578ac7601180741f3622003dbd3235c6fafe35c8 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 8 Apr 2019 11:33:21 -0500 Subject: [PATCH 15/19] don't call the golangci-lint workflow anymore If we're renaming golangci-lint to linter (since it's now our default linter), we no longer have a workflow named golangci-lint, so we shouldn't be calling it or requiring it from other workflows. --- .circleci/config.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index d55e8704b..c7fe6e11b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -132,9 +132,6 @@ workflows: - check-license-headers: requires: - setup - - golangci-lint: - requires: - - setup - test-build-arm: requires: - setup @@ -162,7 +159,6 @@ workflows: requires: - linter - check-license-headers - - golangci-lint - test-golang-1.12 filters: tags: @@ -177,4 +173,3 @@ workflows: - linter - check-license-headers - test-golang-1.12 - - golangci-lint From ae17fcef7ee6d3cc51bb3dcd599c6fa6f9d9541a Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 9 Apr 2019 17:05:31 -0500 Subject: [PATCH 16/19] refix a lint Another test change made an `err :=` fail because it's no longer declaring a new variable, but another one needed the :. Or a patch applied incorrectly. It is a mystery. --- server/server_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/server_test.go b/server/server_test.go index 19b79c6ea..e911b25e6 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -900,7 +900,7 @@ func TestClusterExhaustingConnections(t *testing.T) { return nil }) } - err = eg.Wait() + err := eg.Wait() if err != nil { t.Fatalf("setting lots of shards: %v", err) } @@ -955,7 +955,7 @@ func TestClusterExhaustingConnectionsImport(t *testing.T) { return nil }) } - err := eg.Wait() + err = eg.Wait() if err != nil { t.Fatalf("setting lots of shards: %v", err) } From 302830ed6069198db2f3bfeadaf6219c6bd442cf Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 9 Apr 2019 17:10:44 -0500 Subject: [PATCH 17/19] fix lint in btree_test --- roaring/btree_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/roaring/btree_test.go b/roaring/btree_test.go index 67fd49e68..c12892f62 100644 --- a/roaring/btree_test.go +++ b/roaring/btree_test.go @@ -101,16 +101,16 @@ func (t *tree) dump() string { n = i + 1 } } - f.Format("%sX#%d(%p) n %d:%d {", pref, h, x, x.c, n) + _, _ = f.Format("%sX#%d(%p) n %d:%d {", pref, h, x, x.c, n) a := []interface{}{} for i, v := range x.x[:n] { a = append(a, v.ch) if i != 0 { - f.Format(" ") + _, _ = f.Format(" ") } - f.Format("(C#%d K %v)", handle(v.ch), v.k) + _, _ = f.Format("(C#%d K %v)", handle(v.ch), v.k) } - f.Format("}\n") + _, _ = f.Format("}\n") for _, p := range a { pagedump(p, pref+". ") } @@ -122,14 +122,14 @@ func (t *tree) dump() string { n = i + 1 } } - f.Format("%sD#%d(%p) P#%d N#%d n %d:%d {", pref, h, x, handle(x.p), handle(x.n), x.c, n) + _, _ = f.Format("%sD#%d(%p) P#%d N#%d n %d:%d {", pref, h, x, handle(x.p), handle(x.n), x.c, n) for i, d := range x.d[:n] { if i != 0 { - f.Format(" ") + _, _ = f.Format(" ") } - f.Format("%v:%v", d.k, d.v) + _, _ = f.Format("%v:%v", d.k, d.v) } - f.Format("}\n") + _, _ = f.Format("}\n") } } From fc5fc4151b942ad9b7df0bf8a9625885bd5ffc84 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 9 Apr 2019 17:10:54 -0500 Subject: [PATCH 18/19] add missing error check --- utils_internal_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/utils_internal_test.go b/utils_internal_test.go index 2f21fb04b..76e70fb9e 100644 --- a/utils_internal_test.go +++ b/utils_internal_test.go @@ -360,7 +360,10 @@ func (b bcast) SendTo(to *Node, m Message) error { // Apply the send message to the node. for _, c := range b.t.Clusters { if c.Node.ID == to.ID { - c.mergeClusterStatus(obj) + err := c.mergeClusterStatus(obj) + if err != nil { + return err + } } } b.t.mu.RLock() From 449c853850892762052ca6507a2fda90a988e583 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 16 Apr 2019 11:24:07 -0500 Subject: [PATCH 19/19] address meta-lint or half-baked lint fixes Clean up some spelling and consistency issues for the lint fixes. --- boltdb/attrstore.go | 4 ++-- cluster_internal_test.go | 17 ++++++----------- cmd/root.go | 7 +++++-- ctl/generate_config.go | 2 +- ctl/inspect.go | 2 +- ctl/inspect_test.go | 2 +- field_test.go | 2 +- 7 files changed, 17 insertions(+), 19 deletions(-) diff --git a/boltdb/attrstore.go b/boltdb/attrstore.go index 280275a4d..e8c770fde 100644 --- a/boltdb/attrstore.go +++ b/boltdb/attrstore.go @@ -239,7 +239,7 @@ func (s *attrStore) Blocks() (blocks []pilosa.AttrBlock, err error) { return nil }) if err != nil { - return nil, err + return nil, errors.Wrap(err, "getting blocks") } return blocks, nil } @@ -271,7 +271,7 @@ func (s *attrStore) BlockData(i uint64) (m map[uint64]map[string]interface{}, er return nil }) if err != nil { - return nil, err + return nil, errors.Wrap(err, "getting block data") } return m, nil } diff --git a/cluster_internal_test.go b/cluster_internal_test.go index 84392ffd9..2a557694d 100644 --- a/cluster_internal_test.go +++ b/cluster_internal_test.go @@ -710,8 +710,7 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Multiple nodes, in/not in topology", func(t *testing.T) { tc := NewClusterCluster(0) - err := tc.addNode() - if err != nil { + if err := tc.addNode(); err != nil { t.Fatalf("adding node: %v", err) } node0 := tc.Clusters[0] @@ -736,13 +735,11 @@ func TestCluster_ResizeStates(t *testing.T) { // Expect an error by adding a node not in the topology. expectedError := "host is not in topology: node1" - err = tc.addNode() - if err == nil || err.Error() != expectedError { + if err := tc.addNode(); err == nil || err.Error() != expectedError { t.Errorf("did not receive expected error: %s", expectedError) } - err = tc.addNode() - if err != nil { + if err := tc.addNode(); err != nil { t.Fatalf("adding node: %v", err) } node2 := tc.Clusters[2] @@ -762,14 +759,13 @@ func TestCluster_ResizeStates(t *testing.T) { t.Run("Multiple nodes, with data", func(t *testing.T) { tc := NewClusterCluster(0) - err := tc.addNode() - if err != nil { + if err := tc.addNode(); err != nil { t.Fatalf("adding node: %v", err) } node0 := tc.Clusters[0] // Open TestCluster. - if err = tc.Open(); err != nil { + if err := tc.Open(); err != nil { t.Fatal(err) } @@ -792,8 +788,7 @@ func TestCluster_ResizeStates(t *testing.T) { node0Checksum := node0Fragment.Checksum() // addNode needs to block until the resize process has completed. - err = tc.addNode() - if err != nil { + if err := tc.addNode(); err != nil { t.Fatalf("adding node: %v", err) } node1 := tc.Clusters[1] diff --git a/cmd/root.go b/cmd/root.go index a8f8e6ddc..3ff5c22e4 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -51,11 +51,14 @@ Build Time: ` + pilosa.BuildTime + "\n", } // return "dry run" error if "dry-run" flag is set - if ret, err := cmd.Flags().GetBool("dry-run"); ret && err == nil { + ret, err := cmd.Flags().GetBool("dry-run") + if err != nil { + return fmt.Errorf("problem getting dry-run flag: %v", err) + } + if ret { if cmd.Parent() != nil { return fmt.Errorf("dry run") } - return fmt.Errorf("problem getting dry-run flag: %v", err) } return nil diff --git a/ctl/generate_config.go b/ctl/generate_config.go index 0e59ce097..5a7e0db9f 100644 --- a/ctl/generate_config.go +++ b/ctl/generate_config.go @@ -42,7 +42,7 @@ func (cmd *GenerateConfigCommand) Run(_ context.Context) error { conf := server.NewConfig() ret, err := toml.Marshal(*conf) if err != nil { - return errors.Wrap(err, "unmarshaling default config") + return errors.Wrap(err, "unmarshalling default config") } fmt.Fprintf(cmd.Stdout, "%s\n", ret) return nil diff --git a/ctl/inspect.go b/ctl/inspect.go index 204ffab3b..98222d0de 100644 --- a/ctl/inspect.go +++ b/ctl/inspect.go @@ -72,7 +72,7 @@ func (cmd *InspectCommand) Run(_ context.Context) error { }() // Attach the mmap file to the bitmap. t := time.Now() - fmt.Fprintf(cmd.Stderr, "unmarshaling bitmap...") + fmt.Fprintf(cmd.Stderr, "unmarshalling bitmap...") bm := roaring.NewBitmap() if err := bm.UnmarshalBinary(data); err != nil { return errors.Wrap(err, "unmarshalling") diff --git a/ctl/inspect_test.go b/ctl/inspect_test.go index 69efe07fa..bb87f894d 100644 --- a/ctl/inspect_test.go +++ b/ctl/inspect_test.go @@ -51,7 +51,7 @@ func TestInspectCommand_Run(t *testing.T) { if err != nil { t.Fatalf("copying data: %v", err) } - if !strings.Contains(buf.String(), "unmarshaling bitmap...") { + if !strings.Contains(buf.String(), "unmarshalling bitmap...") { t.Fatalf("Inspect doesn't work: %s", err) } diff --git a/field_test.go b/field_test.go index 5911abd6a..88a3f3569 100644 --- a/field_test.go +++ b/field_test.go @@ -219,7 +219,7 @@ func TestField_AvailableShards(t *testing.T) { for i := uint64(0); i < 5; i++ { err := f.RemoveAvailableShard(i) if err != nil { - t.Fatalf("removing shard: %v", err) + t.Fatalf("removing shard %d: %v", i, err) } } if diff := cmp.Diff(f.AvailableShards().Slice(), []uint64{0, 2}); diff != "" {