review comments

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

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"
 }

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"`

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)
+
 }