From 9255d43e9a9fb90990a0b7697d1434684c9f6ee4 Mon Sep 17 00:00:00 2001 From: Travis Date: Sat, 9 May 2020 22:26:12 -0500 Subject: [PATCH 1/3] tidy up some of the TODO comments --- client.go | 2 ++ cmd/root.go | 6 +++--- encoding/proto/proto.go | 3 +-- handler.go | 4 +++- logger/filewriter_test.go | 4 ++++ mock/translator.go | 2 -- server.go | 2 -- test/pilosa.go | 3 +++ transaction.go | 5 +++-- translate.go | 10 +++++++--- 10 files changed, 26 insertions(+), 15 deletions(-) diff --git a/client.go b/client.go index 01579b1e8..a07167b67 100644 --- a/client.go +++ b/client.go @@ -44,6 +44,8 @@ type FieldValue struct { // something hasn't been architected correctly. // While I understand that putting the entire Client behind an interface might require this many methods, // I don't want to let it go unquestioned. +// Another note from Travis: I think we eventually want to unify `InternalClient` with the `go-pilosa` client. +// Doing that may obviate the need to refactor this. type InternalClient interface { InternalQueryClient diff --git a/cmd/root.go b/cmd/root.go index f59423232..228d3ed29 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -31,13 +31,13 @@ func NewRootCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command { productName = "Pilosa Enterprise " + pilosa.Version } rc := &cobra.Command{ - Use: "pilosa", + Use: "pilosa", + // TODO: These short/long descriptions could use some updating. Short: "Pilosa - A Distributed In-memory Binary Bitmap Index.", - // TODO - is documentation actually there? Long: `Pilosa is a fast index to turbocharge your database. This binary contains Pilosa itself, as well as common -tools for administering pilosa, importing/exporting data, +tools for administering Pilosa, importing/exporting data, backing up, and more. Complete documentation is available at https://www.pilosa.com/docs/. diff --git a/encoding/proto/proto.go b/encoding/proto/proto.go index 086b5101d..0fef659e0 100644 --- a/encoding/proto/proto.go +++ b/encoding/proto/proto.go @@ -1253,13 +1253,12 @@ func decodeTransactionMessage(pb *internal.TransactionMessage, m *pilosa.Transac } func decodeTransaction(pb *internal.Transaction, trns *pilosa.Transaction) { - trns.ID = pb.ID trns.Active = pb.Active trns.Exclusive = pb.Exclusive trns.Timeout = time.Duration(pb.Timeout) trns.Deadline = time.Unix(0, pb.Deadline) - // TODO m.Stats... once it has anything + // TODO: trns.Stats... once it has anything } // QueryResult types. diff --git a/handler.go b/handler.go index 486b99217..3c40d20ad 100644 --- a/handler.go +++ b/handler.go @@ -57,7 +57,9 @@ type QueryRequest struct { // QueryResponse represent a response from a processed query. type QueryResponse struct { // Result for each top-level query call. - // Can be a Bitmap, Pairs, or uint64. // TODO: this comment is out of date. + // The result type differs depending on the query; types + // include: Row, RowIdentifiers, GroupCounts, SignedRow, + // ValCount, Pair, Pairs, bool, uint64. Results []interface{} // Set of column attribute objects matching IDs returned in Result. diff --git a/logger/filewriter_test.go b/logger/filewriter_test.go index 1004daaad..8351e580d 100644 --- a/logger/filewriter_test.go +++ b/logger/filewriter_test.go @@ -43,6 +43,8 @@ import ( // func TestReopenAppend(t *testing.T) { // TODO fix + // (travis) I have no idea what this TODO is asking for. + // Perhaps use `ioutil.TempFile()`? var fname = "/tmp/foo" // Step 1 -- Create a sample file using normal means @@ -105,6 +107,8 @@ func TestReopenAppend(t *testing.T) { // func TestChangeInode(t *testing.T) { // TODO fix + // (travis) I have no idea what this TODO is asking for. + // Perhaps use `ioutil.TempFile()`? var fname = "/tmp/foo" // Step 1 -- Create a empty sample file diff --git a/mock/translator.go b/mock/translator.go index baf7de524..dc1e5a420 100644 --- a/mock/translator.go +++ b/mock/translator.go @@ -81,12 +81,10 @@ func (s *TranslateStore) EntryReader(ctx context.Context, offset uint64) (pilosa return s.EntryReaderFunc(ctx, offset) } -// TODO: implement this func (s *TranslateStore) WriteTo(w io.Writer) (int64, error) { return 0, nil } -// TODO: implement this func (s *TranslateStore) ReadFrom(r io.Reader) (int64, error) { return 0, nil } diff --git a/server.go b/server.go index d3926ab23..287fba0af 100644 --- a/server.go +++ b/server.go @@ -88,7 +88,6 @@ type Server struct { // nolint: maligned } // Holder returns the holder for server. -// TODO: have this return an interface for Holder instead of concrete object? func (s *Server) Holder() *Holder { return s.holder } @@ -1159,7 +1158,6 @@ func countOpenFiles() (int, error) { lines := strings.Split(string(out), strconv.Itoa(pid)) return len(lines), nil case "windows": - // TODO: count open file handles on windows return 0, errors.New("countOpenFiles() on Windows is not supported") default: return 0, errors.New("countOpenFiles() on this OS is not supported") diff --git a/test/pilosa.go b/test/pilosa.go index 6963f635f..635e0f3d9 100644 --- a/test/pilosa.go +++ b/test/pilosa.go @@ -298,6 +298,9 @@ func (c Cluster) ImportBits(t testing.TB, index, field string, rowcols [][2]uint t.Fatalf("getting shard nodes: %v", err) } // TODO won't be necessary to do all nodes once that works hits + // (travis) this TODO is not clear to me, but I think it's + // suggesting that elsewhere we would support importing to a + // single node, regardless of where the data ends up. for _, node := range nodes { for _, com := range c { if com.API.Node().ID != node.ID { diff --git a/transaction.go b/transaction.go index 1e142ccb2..ba7063907 100644 --- a/transaction.go +++ b/transaction.go @@ -42,8 +42,9 @@ type Transaction struct { // Timeout is the minimum idle time for which this transaction should continue to exist. Timeout time.Duration `json:"timeout"` - // Deadline is calculated from Timeout. TODO reset deadline each time there is activity on the transaction. (we can't do this until there is some method of associating a request/call with a transaction) - // time there is activity on the transaction. + // Deadline is calculated from Timeout. TODO reset deadline each time there is activity + // on the transaction. (we can't do this until there is some method of associating a + // request/call with a transaction) Deadline time.Time `json:"deadline"` // Stats track statistics for the transaction. Not yet used. diff --git a/translate.go b/translate.go index 6088774c2..a01d7f2e2 100644 --- a/translate.go +++ b/translate.go @@ -414,14 +414,18 @@ func (s *InMemTranslateStore) EntryReader(ctx context.Context, offset uint64) (T return newInMemTranslateEntryReader(ctx, s, offset), nil } -// TODO: implement this -// WriteTo writes the contents of the store to the writer. +// WriteTo ensures that the TranslateStore implements io.WriterTo. +// TODO: It's not important that this be implemented. It would really +// only be necessary if we wanted to test cluster resizing while using +// an in-memory translate store. func (s *InMemTranslateStore) WriteTo(w io.Writer) (int64, error) { return 0, nil } -// TODO: implement this // ReadFrom ensures that the TranslateStore implements io.ReaderFrom. +// TODO: It's not important that this be implemented. It would really +// only be necessary if we wanted to test cluster resizing while using +// an in-memory translate store. func (s *InMemTranslateStore) ReadFrom(r io.Reader) (int64, error) { return 0, nil } From d546b8ac015f423a6f469a2543fd1d4e0b8aff86 Mon Sep 17 00:00:00 2001 From: Travis Date: Sun, 10 May 2020 18:50:05 -0500 Subject: [PATCH 2/3] use pilosa.ErrNotImplemented for unused interface implementations --- translate.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/translate.go b/translate.go index a01d7f2e2..ea268bb68 100644 --- a/translate.go +++ b/translate.go @@ -415,19 +415,19 @@ func (s *InMemTranslateStore) EntryReader(ctx context.Context, offset uint64) (T } // WriteTo ensures that the TranslateStore implements io.WriterTo. -// TODO: It's not important that this be implemented. It would really +// It's not important that this be implemented. It would really // only be necessary if we wanted to test cluster resizing while using // an in-memory translate store. func (s *InMemTranslateStore) WriteTo(w io.Writer) (int64, error) { - return 0, nil + return 0, ErrNotImplemented } // ReadFrom ensures that the TranslateStore implements io.ReaderFrom. -// TODO: It's not important that this be implemented. It would really +// It's not important that this be implemented. It would really // only be necessary if we wanted to test cluster resizing while using // an in-memory translate store. func (s *InMemTranslateStore) ReadFrom(r io.Reader) (int64, error) { - return 0, nil + return 0, ErrNotImplemented } // MaxID returns the highest identifier in the store. From bc8244a58121219cfea6482dcff74b7b056ae364 Mon Sep 17 00:00:00 2001 From: Travis Date: Sun, 10 May 2020 19:18:54 -0500 Subject: [PATCH 3/3] well, put the TODO back, just in a different place --- translate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/translate.go b/translate.go index ea268bb68..157c05e2a 100644 --- a/translate.go +++ b/translate.go @@ -419,7 +419,7 @@ func (s *InMemTranslateStore) EntryReader(ctx context.Context, offset uint64) (T // only be necessary if we wanted to test cluster resizing while using // an in-memory translate store. func (s *InMemTranslateStore) WriteTo(w io.Writer) (int64, error) { - return 0, ErrNotImplemented + return 0, nil // TODO: try to use ErrNotImplemented } // ReadFrom ensures that the TranslateStore implements io.ReaderFrom. @@ -427,7 +427,7 @@ func (s *InMemTranslateStore) WriteTo(w io.Writer) (int64, error) { // only be necessary if we wanted to test cluster resizing while using // an in-memory translate store. func (s *InMemTranslateStore) ReadFrom(r io.Reader) (int64, error) { - return 0, ErrNotImplemented + return 0, nil // TODO: try to use ErrNotImplemented } // MaxID returns the highest identifier in the store.