From 6cc5d198ee4815e0b19889c06bcac632cdee78ea Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 4 Feb 2022 11:31:53 -0600 Subject: [PATCH] remove unused stuff and fix a bunch of random staticcheck issues sorry... once I saw, I couldn't unsee --- api_test.go | 4 +- client/batch_test.go | 22 +------- cmd/server_test.go | 23 +++++++-- dbshard.go | 19 ------- fragment.go | 17 +----- fragment_internal_test.go | 8 --- holder_internal_test.go | 19 ------- http_handler_internal_test.go | 33 ++++++------ index_internal_test.go | 11 ---- internal/clustertests/cluster_test.go | 3 ++ internal_client.go | 8 --- rbf/db.go | 1 - server/server.go | 3 ++ txfactory.go | 74 +-------------------------- util.go | 64 ----------------------- 15 files changed, 46 insertions(+), 263 deletions(-) diff --git a/api_test.go b/api_test.go index 29e374234..cd2595064 100644 --- a/api_test.go +++ b/api_test.go @@ -1365,7 +1365,9 @@ func TestVariousApiTranslateCalls(t *testing.T) { if err != nil { t.Fatalf("%v: could not create test index", err) } - _, err = idx.CreateFieldIfNotExistsWithOptions("field", &pilosa.FieldOptions{Keys: false}) + if _, err = idx.CreateFieldIfNotExistsWithOptions("field", &pilosa.FieldOptions{Keys: false}); err != nil { + t.Fatalf("creating field: %v", err) + } t.Run("translateIndexDbOnNilIndex", func(t *testing.T) { err := api.TranslateIndexDB(context.Background(), "nonExistentIndex", 0, r) diff --git a/client/batch_test.go b/client/batch_test.go index 8823a5099..3436ec1fc 100644 --- a/client/batch_test.go +++ b/client/batch_test.go @@ -359,7 +359,7 @@ func testTrimNull(t *testing.T, c *test.Cluster, client *Client) { t.Fatalf("querying: %v", err) } for i, result := range resp.Results() { - if 1 == i { + if i == 1 { if !reflect.DeepEqual(result.Row().Columns, []uint64(nil)) { t.Errorf("expected %#v for %d, but got %#v", []uint64(nil), i, result.Row().Columns) } @@ -1187,26 +1187,6 @@ outer: return nil } -func isPermutationOfInt(one, two []uint64) error { - if len(one) != len(two) { - return errors.Errorf("different lengths %d and %d", len(one), len(two)) - } -outer: - for _, vOne := range one { - for j, vTwo := range two { - if vOne == vTwo { - two = append(two[:j], two[j+1:]...) - continue outer - } - } - return errors.Errorf("%d in one but not two", vOne) - } - if len(two) != 0 { - return errors.Errorf("vals in two but not one: %v", two) - } - return nil -} - func TestQuantizedTime(t *testing.T) { cases := []struct { name string diff --git a/cmd/server_test.go b/cmd/server_test.go index f2d3cf057..91263c0ed 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -3,10 +3,12 @@ package cmd_test import ( "fmt" + "os" "strings" "testing" "time" + "github.com/felixge/fgprof" "github.com/molecula/featurebase/v3/cmd" _ "github.com/molecula/featurebase/v3/test" "github.com/molecula/featurebase/v3/testhook" @@ -23,12 +25,11 @@ func TestServerHelp(t *testing.T) { } // I have no idea why the linter in ci is complaining about this being unused. -func nextPort() string { //nolint:unused +func nextPort() string { return fmt.Sprintf(`"localhost:%d"`, 0) } func TestServerConfig(t *testing.T) { - t.Skip("pilosa hosts config (cmd.Server.Config.Cluster.Hosts and brethren) is test only and will go away with high probability. skip for now.") actualDataDir, err := testhook.TempDir(t, "") failErr(t, err, "making data dir") logFile, err := testhook.TempFile(t, "") @@ -106,7 +107,7 @@ func TestServerConfig(t *testing.T) { }, // TEST 2 { - args: []string{"server", "--log-path", logFile.Name(), "--cluster.disabled", "true", "--translation.map-size", "100000"}, + args: []string{"server", "--log-path", logFile.Name(), "--translation.map-size", "100000"}, env: map[string]string{}, cfgFileContent: ` bind = "localhost:19444" @@ -175,7 +176,9 @@ func TestServerConfig(t *testing.T) { } } func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { - t.Skip("pilosa hosts config (cmd.Server.Config.Cluster.Hosts and brethren) is test only and will go away with high probability. skip for now.") + // if you don't pass an empty dir as data-dir it will use the + // default... which might be full of data and cause the test to + // run super slow. actualDataDir, err := testhook.TempDir(t, "") failErr(t, err, "making data dir") @@ -203,6 +206,7 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { cfgFileContent: ` bind = ` + nextPort() + ` bind-grpc = ` + nextPort() + ` + data-dir = "` + actualDataDir + `" `, validation: func() error { v := validator{} @@ -218,6 +222,7 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { cfgFileContent: ` bind = ` + nextPort() + ` bind-grpc = ` + nextPort() + ` + data-dir = "` + actualDataDir + `" `, validation: func() error { v := validator{} @@ -228,7 +233,11 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { }, }, } - + out, err := os.Create("myprof.prof") + if err != nil { + t.Fatalf("creating prof file: %v", err) + } + stop := fgprof.Start(out, fgprof.FormatPprof) // run server tests for i, test := range tests { t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) { @@ -257,4 +266,8 @@ func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { test.reset() }) } + err = stop() + if err != nil { + t.Fatalf("stopping profile: %v", err) + } } diff --git a/dbshard.go b/dbshard.go index 7e7387c68..6c1945fdb 100644 --- a/dbshard.go +++ b/dbshard.go @@ -236,12 +236,6 @@ func newShardSet() *shardSet { shardsMap: make(map[uint64]bool), } } -func newShardSetFromMap(m map[uint64]bool) *shardSet { - return &shardSet{ - shardsMap: m, - shardsVer: 1, - } -} func (per *DBPerShard) LoadExistingDBs() (err error) { idxs := per.holder.Indexes() @@ -582,19 +576,6 @@ func (vs *FieldView2Shards) getViewsForField(field string) map[string]*shardSet return vs.m[field] } -func (vs *FieldView2Shards) has(field, view string, shard uint64) bool { - vw, ok := vs.m[field] - if !ok { - return false - } - ss, ok := vw[view] - if !ok { - return false - } - shardMap := ss.CloneMaybe() - return shardMap[shard] -} - func (vs *FieldView2Shards) addViewShardSet(fv txkey.FieldView, ss *shardSet) { f, ok := vs.m[fv.Field] diff --git a/fragment.go b/fragment.go index 3e8e45741..54d68eed9 100644 --- a/fragment.go +++ b/fragment.go @@ -19,9 +19,7 @@ import ( "strconv" "strings" "sync" - "syscall" "time" - "unsafe" "github.com/cespare/xxhash" "github.com/gogo/protobuf/proto" @@ -127,9 +125,6 @@ type fragment struct { // parent holder holder *Holder - // File-backed storage - storage *roaring.Bitmap - // Cache for row counts. CacheType string // passed in by field @@ -153,8 +148,6 @@ type fragment struct { mutexVector vector stats stats.StatsClient - - bitmapInfo *roaring.BitmapInfo } // newFragment returns a new instance of fragment. @@ -2331,7 +2324,7 @@ func (f *fragment) doImportRoaring(ctx context.Context, tx Tx, data []byte, clea f.mu.RLock() defer f.mu.RUnlock() rowSize := uint64(1 << shardVsContainerExponent) - span, ctx := tracing.StartSpanFromContext(ctx, "importRoaring.ImportRoaringBits") + span, _ := tracing.StartSpanFromContext(ctx, "importRoaring.ImportRoaringBits") defer span.Finish() var rowSet map[uint64]int @@ -3308,14 +3301,6 @@ func bitsToRoaringData(ps pairSet) ([]byte, error) { return buf.Bytes(), nil } -func madvise(b []byte, advice int) error { // nolint: unparam - _, _, err := syscall.Syscall(syscall.SYS_MADVISE, uintptr(unsafe.Pointer(&b[0])), uintptr(len(b)), uintptr(advice)) - if err != 0 { - return err - } - return nil -} - // pairSet is a list of equal length row and column id lists. type pairSet struct { rowIDs []uint64 diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 8953e113d..6fd803954 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -3879,14 +3879,6 @@ func TestFragmentRowIterator_WithTxCommit(t *testing.T) { }) } -func randPositions(n int, r *rand.Rand) []uint64 { - ret := make([]uint64, n) - for i := 0; i < n; i++ { - ret[i] = uint64(r.Int63n(ShardWidth)) - } - return ret -} - func TestFragmentPositionsForValue(t *testing.T) { f, _, _ := mustOpenFragment(t, "i", "f", "v", 0, CacheTypeNone) defer f.Clean(t) diff --git a/holder_internal_test.go b/holder_internal_test.go index 4204dddc1..e18704ac8 100644 --- a/holder_internal_test.go +++ b/holder_internal_test.go @@ -2,28 +2,9 @@ package pilosa import ( - "testing" - "github.com/molecula/featurebase/v3/disco" ) -func testSetBit(t *testing.T, h *Holder, index, field string, rowID, columnID uint64) { - - idx, err := h.CreateIndexIfNotExists(index, IndexOptions{}) - if err != nil { - t.Fatalf("creating index: %v", err) - } - - f, err := idx.CreateFieldIfNotExists(field, OptFieldTypeDefault()) - if err != nil { - t.Fatalf("setting bit: %v", err) - } - _, err = f.SetBit(nil, rowID, columnID, nil) - if err != nil { - t.Fatalf("setting bit: %v", err) - } -} - // mustHolderConfig sets up a default holder config for tests. func mustHolderConfig() *HolderConfig { cfg := DefaultHolderConfig() diff --git a/http_handler_internal_test.go b/http_handler_internal_test.go index faee6173a..455a40c64 100644 --- a/http_handler_internal_test.go +++ b/http_handler_internal_test.go @@ -7,7 +7,6 @@ import ( "encoding/json" "io/ioutil" "net/http" - gohttp "net/http" "net/http/httptest" "net/url" "os" @@ -188,7 +187,7 @@ func readResponse(w *httptest.ResponseRecorder) ([]byte, error) { func TestAuthentication(t *testing.T) { type evaluate func(w *httptest.ResponseRecorder, data []byte) - type endpoint func(w gohttp.ResponseWriter, r *gohttp.Request) + type endpoint func(w http.ResponseWriter, r *http.Request) var ( ClientId = "e9088663-eb08-41d7-8f65-efb5f54bbb71" ClientSecret = "DEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF" @@ -252,7 +251,7 @@ func TestAuthentication(t *testing.T) { } expiredToken = "Bearer " + expiredToken - validCookie := &gohttp.Cookie{ + validCookie := &http.Cookie{ Name: "molecula-chip", Value: token.AccessToken, Path: "/", @@ -273,7 +272,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` method string yamlData string token string - cookie *gohttp.Cookie + cookie *http.Cookie handler endpoint fn evaluate }{ @@ -334,7 +333,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` handler: h.handleCheckAuthentication, fn: func(w *httptest.ResponseRecorder, data []byte) { // no valid token in header == Unauthorized - if w.Result().StatusCode != gohttp.StatusUnauthorized { + if w.Result().StatusCode != http.StatusUnauthorized { t.Errorf("expected http code 401, got: %+v", w.Result().StatusCode) } }, @@ -376,7 +375,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` token: "", handler: h.handleUserInfo, fn: func(w *httptest.ResponseRecorder, data []byte) { - if got := w.Result().StatusCode; got != gohttp.StatusForbidden { + if got := w.Result().StatusCode; got != http.StatusForbidden { t.Errorf("expected 403, got %v", got) } }, @@ -484,7 +483,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` path: "/index/{index}/query", kind: "middleware", cookie: validCookie, - handler: func(w gohttp.ResponseWriter, r *gohttp.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { f := hOff.chkAuthZ(hOff.handlePostQuery, authz.Admin) f(w, r) }, @@ -498,9 +497,9 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` name: "MW-CreateIndexInsufficientPerms", path: "/index/abcd", kind: "bearer", - method: gohttp.MethodPost, + method: http.MethodPost, token: validToken, - handler: func(w gohttp.ResponseWriter, r *gohttp.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { h := h var p authz.GroupPermissions if err := p.ReadPermissionsFile(strings.NewReader(permissions1)); err != nil { @@ -512,7 +511,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` f(w, r) }, fn: func(w *httptest.ResponseRecorder, data []byte) { - if got, want := w.Result().StatusCode, gohttp.StatusForbidden; got != want { + if got, want := w.Result().StatusCode, http.StatusForbidden; got != want { t.Errorf("expected %v, got %v", want, got) } }, @@ -524,13 +523,13 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` path: "/index/{index}/query", kind: "bearer", token: validToken, - handler: func(w gohttp.ResponseWriter, r *gohttp.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { h := h f := h.chkAuthZ(h.handlePostQuery, authz.Write) f(w, r) }, fn: func(w *httptest.ResponseRecorder, data []byte) { - if got, want := w.Result().StatusCode, gohttp.StatusInternalServerError; got != want { + if got, want := w.Result().StatusCode, http.StatusInternalServerError; got != want { t.Errorf("expected %v, got %v", want, got) } }, @@ -540,7 +539,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` path: "/index/{index}/query", kind: "bearer", token: validToken, - handler: func(w gohttp.ResponseWriter, r *gohttp.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { h := h var p authz.GroupPermissions if err := p.ReadPermissionsFile(strings.NewReader(permissions1)); err != nil { @@ -551,7 +550,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` f(w, r) }, fn: func(w *httptest.ResponseRecorder, data []byte) { - if got, want := w.Result().StatusCode, gohttp.StatusBadRequest; got != want { + if got, want := w.Result().StatusCode, http.StatusBadRequest; got != want { t.Errorf("expected %v, got: %+v", want, got) } }, @@ -562,7 +561,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` switch test.kind { case "type1", "middleware": t.Run(test.name, func(t *testing.T) { - r := httptest.NewRequest(gohttp.MethodGet, test.path, nil) + r := httptest.NewRequest(http.MethodGet, test.path, nil) w := httptest.NewRecorder() if test.cookie != nil { r.AddCookie(test.cookie) @@ -576,7 +575,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` }) case "type2": t.Run(test.name, func(t *testing.T) { - r := httptest.NewRequest(gohttp.MethodGet, test.path, nil) + r := httptest.NewRequest(http.MethodGet, test.path, nil) w := httptest.NewRecorder() r.Form = url.Values{} r.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -594,7 +593,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` case "bearer": t.Run(test.name, func(t *testing.T) { if test.method == "" { - test.method = gohttp.MethodGet + test.method = http.MethodGet } r := httptest.NewRequest(test.method, test.path, nil) w := httptest.NewRecorder() diff --git a/index_internal_test.go b/index_internal_test.go index a36a2475a..ca953d458 100644 --- a/index_internal_test.go +++ b/index_internal_test.go @@ -28,14 +28,3 @@ func mustOpenIndex(tb testing.TB, opt IndexOptions) *Index { return index } - -// reopen closes the index and reopens it. -func (i *Index) reopen() error { - if err := i.Close(); err != nil { - return err - } - if err := i.Open(); err != nil { - return err - } - return nil -} diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index c3548589d..a54f6256b 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -63,6 +63,9 @@ func GetAuthToken(t *testing.T) string { ClientSecret, Key, ) + if err != nil { + t.Fatalf("NewAuth: %v", err) + } // make a valid token tkn := jwt.New(jwt.SigningMethodHS256) diff --git a/internal_client.go b/internal_client.go index f225474e8..fa1bc6653 100644 --- a/internal_client.go +++ b/internal_client.go @@ -1858,14 +1858,6 @@ func forwardAuthHeader(b bool) executeRequestOption { } } -type nopCloser struct { - *bytes.Reader -} - -func (n nopCloser) Close() error { - return nil -} - // executeRequest executes the given request and checks the Response. For // responses with non-2XX status, the body is read and closed, and an error is // returned. If the error is nil, the caller must ensure that the response body diff --git a/rbf/db.go b/rbf/db.go index f27598467..a6c248e5b 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -704,7 +704,6 @@ func (db *DB) afterCurrentTx(callback func()) { defer db.mu.Unlock() txw.callback() }() - return } // removeTx removes an active transaction from the database. it obtains diff --git a/server/server.go b/server/server.go index 2bc95c354..697816f16 100644 --- a/server/server.go +++ b/server/server.go @@ -571,6 +571,9 @@ func (m *Command) SetupServer() error { OptGRPCServerPerm(&p), OptGRPCServerQueryLogger(m.queryLogger), ) + if err != nil { + return errors.Wrap(err, "getting grpcServer") + } m.Handler, err = pilosa.NewHandler( pilosa.OptHandlerAllowedOrigins(m.Config.Handler.AllowedOrigins), diff --git a/txfactory.go b/txfactory.go index 30f1a5efd..58134a6bb 100644 --- a/txfactory.go +++ b/txfactory.go @@ -5,8 +5,6 @@ import ( "fmt" "os" "path" - "path/filepath" - "strconv" "strings" "sync" @@ -836,73 +834,6 @@ func (ty txtype) String() string { return "" } -// fragmentSpecFromRoaringPath takes a path releative to the -// index directory, not including the name of the index itself. -// The path should not start with the path separator sep ('/' or '\\') rune. -func fragmentSpecFromRoaringPath(path string) (field, view string, shard uint64, err error) { - if len(path) == 0 { - err = fmt.Errorf("fragmentSpecFromRoaringPath error: path '%v' too short", path) - return - } - if path[:1] == sep { - err = fmt.Errorf("fragmentSpecFromRoaringPath error: path '%v' cannot start with separator '%v'; must be relative to the index base directory", path, sep) - return - } - - // sample path: - // field view shard - // fields/myfield/views/standard/fragments/0 - s := strings.Split(path, "/") - n := len(s) - if n != 6 { - err = fmt.Errorf("len(s)=%v, but expected 5. path='%v'", n, path) - return - } - field = s[1] - view = s[3] - shard, err = strconv.ParseUint(s[5], 10, 64) - if err != nil { - err = fmt.Errorf("fragmentSpecFromRoaringPath(path='%v') could not parse shard '%v' as uint: '%v'", path, s[5], err) - } - return -} - -// listFilesUnderDir returns the paths of files found under directory root. -// If includeRoot is true, it returns the full path, otherwise paths are relative to root. -// If requriedSuffix is supplied, the returned file paths will end in that, -// and any other files found during the walk of the directory tree will be ignored. -// If ignoreEmpty is true, files of size 0 will be excluded. -func listFilesUnderDir(root string, includeRoot bool, requiredSuffix string, ignoreEmpty bool) (files []string, err error) { - if !dirExists(root) { - return nil, fmt.Errorf("listFilesUnderDir error: root directory '%v' not found", root) - } - n := len(root) + 1 - if includeRoot { - n = 0 - } - err = filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if len(path) < n { - // ignore - } else { - if info == nil { - vprint.PanicOn(fmt.Sprintf("info was nil for path = '%v'", path)) - } - if info.IsDir() { - // skip directories. - } else { - if ignoreEmpty && info.Size() == 0 { - return nil - } - if requiredSuffix == "" || strings.HasSuffix(path, requiredSuffix) { - files = append(files, path[n:]) - } - } - } - return nil - }) - return -} - func dirExists(name string) bool { fi, err := os.Stat(name) if err != nil { @@ -925,10 +856,7 @@ func fileSize(name string) (int64, error) { var _ = anyGlobalDBWrappersStillOpen // happy linter func anyGlobalDBWrappersStillOpen() bool { - if globalRbfDBReg.Size() != 0 { - return true - } - return false + return globalRbfDBReg.Size() != 0 } func (f *TxFactory) hasRBF() bool { diff --git a/util.go b/util.go index ad7397688..eb9f958ce 100644 --- a/util.go +++ b/util.go @@ -4,13 +4,8 @@ package pilosa // util.go: a place for generic, reusable utilities. import ( - "os" "reflect" - "syscall" "time" - - "github.com/molecula/featurebase/v3/roaring" - "github.com/pkg/errors" ) // LeftShifted16MaxContainerKey is 0xffffffffffff0000. It is similar @@ -43,65 +38,6 @@ func NilInside(iface interface{}) bool { func highbits(v uint64) uint64 { return v >> 16 } func lowbits(v uint64) uint16 { return uint16(v & 0xFFFF) } -// called by Holder.hasRoaringData() -func roaringFragmentHasData(path string, index, field, view string, shard uint64) (hasData bool, err error) { - - var info roaring.BitmapInfo - _ = info - var f *os.File - f, err = os.Open(path) - if err != nil { - return - } - - var fi os.FileInfo - fi, err = f.Stat() - if err != nil { - return - } - - // Memory map the file. - data, err := syscall.Mmap(int(f.Fd()), 0, int(fi.Size()), syscall.PROT_READ, syscall.MAP_SHARED) - if err != nil { - err = errors.Wrap(err, "mmapping") - return - } - defer func() { - err = syscall.Munmap(data) - if err != nil { - err = errors.Wrap(err, "roaringFragmentHasData: munmap failed") - } - err = f.Close() - if err != nil { - err = errors.Wrap(err, "roaringFragmentHasData f.Close() in defer") - } - }() - - // Attach the mmap file to the bitmap. - var rbm *roaring.Bitmap - rbm, _, err = roaring.InspectBinary(data, true, &info) - if err != nil { - err = errors.Wrap(err, "inspecting") - return - } - - if info.ContainerCount > 0 { - return true, nil - } - if info.Ops > 0 { - return true, nil - } - - citer, found := rbm.Containers.Iterator(0) - _ = found - - for citer.Next() { - return true, nil - } - - return -} - // GetLoopProgress returns the estimated remaining time to iterate through some // items as well as the loop completion percentage with the following // parameters: