From 51bcded0bfd67a43bc67c78d78759ebbfb234c0d Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 28 Aug 2017 11:03:19 -0700 Subject: [PATCH] review comments Signed-off-by: David Lawrence (github: endophage) --- server/storage/rethinkdb.go | 23 +++++++++++++++++++---- server/storage/sql_models.go | 2 +- server/storage/storage_test.go | 19 +++++++++++-------- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/server/storage/rethinkdb.go b/server/storage/rethinkdb.go index f57e290a11..1acd59e77a 100644 --- a/server/storage/rethinkdb.go +++ b/server/storage/rethinkdb.go @@ -14,6 +14,11 @@ import ( "gopkg.in/dancannon/gorethink.v3" ) +// RethinkDB has eventual consistency. This represents a 60 second blackout +// period of the most recent changes in the changefeed which will not be +// returned while the eventual consistency works itself out. +// It's a var not a const so that the tests can turn it down to zero rather +// than have to include a sleep. var blackoutTime = 60 // RDBTUFFile is a TUF file record @@ -342,7 +347,8 @@ func (rdb RethinkDB) writeChange(gun string, version int, sha256, category strin return err } -// GetChanges is not implemented for RethinkDB +// GetChanges returns up to pageSize changes starting from changeID. It uses the +// blackout to account for RethinkDB's eventual consistency model func (rdb RethinkDB) GetChanges(changeID string, pageSize int, filterName string) ([]Change, error) { var ( lower, upper, bound []interface{} @@ -386,6 +392,13 @@ func (rdb RethinkDB) GetChanges(changeID string, pageSize int, filterName string changes := make([]Change, 0, pageSize) + // Between returns a slice of results from the rethinkdb table. + // The results are ordered using BetweenOpts.Index, which will + // default to the index of the immediately preceding OrderBy. + // The lower and upper are the start and end points for the slice + // and the Left/RightBound values determine whether the lower and + // upper values are included in the result per normal set semantics + // of "open" and "closed" res, err := gorethink.DB(rdb.dbName). Table(Change{}.TableName()). OrderBy(order). @@ -414,10 +427,12 @@ func (rdb RethinkDB) GetChanges(changeID string, pageSize int, filterName string return changes, res.All(&changes) } +// bound creates the correct boundary based in the index that should be used for +// querying the changefeed. func (rdb RethinkDB) bound(changeID, filterName string) ([]interface{}, string) { - term := gorethink.DB(rdb.dbName).Table(Change{}.TableName()).Get(changeID).Field("created_at") + createdAtTerm := gorethink.DB(rdb.dbName).Table(Change{}.TableName()).Get(changeID).Field("created_at") if filterName != "" { - return []interface{}{filterName, term, changeID}, "rdb_gun_created_at_id" + return []interface{}{filterName, createdAtTerm, changeID}, "rdb_gun_created_at_id" } - return []interface{}{term, changeID}, "rdb_created_at_id" + return []interface{}{createdAtTerm, changeID}, "rdb_created_at_id" } diff --git a/server/storage/sql_models.go b/server/storage/sql_models.go index a7d9ee0235..f57caeb3d7 100644 --- a/server/storage/sql_models.go +++ b/server/storage/sql_models.go @@ -34,7 +34,7 @@ func (g TUFFile) TableName() string { // SQLChange defines the the fields required for an object in the changefeed type SQLChange struct { - ID uint `gorm:"primary_key" sql:";not null" json:",string"` + ID uint `gorm:"primary_key" sql:"not null" json:",string"` CreatedAt time.Time GUN string `gorm:"column:gun" sql:"type:varchar(255);not null"` Version int `sql:"not null"` diff --git a/server/storage/storage_test.go b/server/storage/storage_test.go index 9e8bb69dbf..06579e8d6a 100644 --- a/server/storage/storage_test.go +++ b/server/storage/storage_test.go @@ -386,14 +386,6 @@ func testGetChanges(t *testing.T, s MetaStore) { require.NoError(t, err) require.Len(t, c, 0) - //c, err = s.GetChanges(full[7].ID, -4, "") - //require.NoError(t, err) - //require.Len(t, c, 4) - //for i := 0; i < 4; i++ { - // require.Equal(t, "busybox", c[i].GUN) - // require.Equal(t, i+1, c[i].Version) - //} - c, err = s.GetChanges(full[6].ID, -4, "") require.NoError(t, err) require.Len(t, c, 4) @@ -437,6 +429,16 @@ func testGetChanges(t *testing.T, s MetaStore) { require.NoError(t, err) require.Equal(t, before, after) + _, err1 := s.GetChanges("1000", 0, "") + _, err2 := s.GetChanges("doesn't exist", 0, "") + if _, ok := s.(*RethinkDB); ok { + require.Error(t, err1) + require.Error(t, err2) + } else { + require.NoError(t, err1) + require.Error(t, err2) + } + // do a deletion and check is shows up. require.NoError(t, s.Delete("alpine")) c, err = s.GetChanges("-1", -1, "") @@ -453,4 +455,5 @@ func testGetChanges(t *testing.T, s MetaStore) { require.Len(t, c, 2) require.NotEqual(t, changeCategoryDeletion, c[0].Category) require.NotEqual(t, "alpine", c[0].GUN) + }