changefeed for rethinkdb #1214

endophage · 2017-08-24T23:25:46Z

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

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

riyazdf

I'm still digesting other parts of this PR but here's some preliminary feedback. Thanks for adding this @endophage!

riyazdf · 2017-08-28T16:39:53Z

server/storage/memory.go

@@ -200,7 +200,7 @@ func (st *MemStorage) Delete(gun data.GUN) error {
 	}
 	delete(st.checksums, gun.String())
 	c := Change{
-		ID:        uint(len(st.changes) + 1),
+		ID:        strconv.Itoa(len(st.changes) + 1),


why is this change necessary? Is this intended to be signed? Same as above.

While it was just SQL backends, we could get away with telling SQL (and Go) that ID was an int, but in the JSON tag saying it was a string. However, the IDs from RethinkDB are UUID strings, and I want strings to be the external data type as the ID is meant to be opaque to users. That necessitated some tweaks to have an internal int type for the GORM backends and converting where necessary.

got it - thanks for the clarification!

riyazdf · 2017-08-28T16:40:21Z

server/storage/rethinkdb.go

@@ -15,6 +14,8 @@ import (
 	"gopkg.in/dancannon/gorethink.v3"
 )

+var blackoutTime = 60


can you please add a comment for what this intended for?

riyazdf · 2017-08-28T16:46:02Z

server/storage/rethinkdb.go

+
+// Change defines the the fields required for an object in the changefeed
+type Change struct {
+	ID        string    `gorethink:"id,omitempty"`


I'm no database expert, but is there a reason for this ID being a string vs. the mysql version being a uint?

RethinkDB IDs are UUID strings

riyazdf · 2017-08-28T16:53:47Z

server/storage/rethinkdb.go

+	// empty string is the zero value for tsChecksum in the RDBTUFFile struct.
+	// Therefore we can just call through to updateCurrentWithTSChecksum passing
+	// "" for the tsChecksum value.
+	if err := rdb.updateCurrentWithTSChecksum(gun.String(), "", update); err != nil {


Why wouldn't we want to keep around the TS checksum for other lookup functionality?

This just DRYs out code, I don't think it changed functionality. Did I miss something?

ah, yes you're right that no functionality has changed. TSchecksum was omitted from this write

riyazdf · 2017-08-28T17:00:03Z

server/storage/rethinkdb.go

@@ -294,7 +324,100 @@ func (rdb RethinkDB) CheckHealth() error {
 	return nil
 }

+func (rdb RethinkDB) writeChange(gun string, version int, sha256, category string) error {
+	now := time.Now()
+	ch := Change{


do we need to specify ID?

No, autoassigned UUID from rethinkdb

riyazdf · 2017-08-28T17:00:40Z

server/storage/rethinkdb.go

+	).RunWrite(rdb.sess)
+	return err
+}
+
 // GetChanges is not implemented for RethinkDB


update function docstring

riyazdf · 2017-08-28T17:01:42Z

server/storage/rethinkdb.go

+	var (
+		lower, upper, bound []interface{}
+		idx                 = "rdb_created_at_id"
+		max                 = []interface{}{gorethink.Now().Sub(blackoutTime), gorethink.MaxVal}


is it worth introducing blackoutTime to rethinkdb when it doesn't seem to be implemented in mysql?

Yes, it deals with the eventual consistency of RethinkDB. MySQL doesn't have an eventual consistency issue.

from IRL discussion: would be helpful to leave a comment about the compound min/max corresponding to the behavior of Between operating on an index

riyazdf · 2017-08-28T17:02:47Z

server/storage/storage_test.go

-		require.Equal(t, "busybox", c[i].GUN)
-		require.Equal(t, i+1, c[i].Version)
-	}
+	//c, err = s.GetChanges(full[7].ID, -4, "")


either bring back this test or remove?

was planning to delete 👍

riyazdf · 2017-08-28T17:23:51Z

server/storage/rethinkdb.go

+		min = append([]interface{}{filterName}, min...)
+	}
+
+	switch changeID {


would it be helpful to refactor the MySQL GetChanges to factor out a helper that handles the changeID and pageSize logic? It might be helpful to consolidate that as well, so that the functionality between the two backends is as identical as possible.

Here's the code I had in mind: https://github.com/docker/notary/blob/master/server/storage/sqldb.go#L279-L294

Going to leave this for a future PR due to time constraints.

riyazdf · 2017-08-28T17:26:20Z

server/storage/rethinkdb.go

+	return changes, res.All(&changes)
+}
+
+func (rdb RethinkDB) bound(changeID, filterName string) ([]interface{}, string) {


could you please add a function docstring here?

riyazdf · 2017-08-28T17:27:37Z

server/storage/rethinkdb.go

+	}
+
+	switch changeID {
+	case "0", "-1":


is there a semantic difference between changeID == 0 and changeID == -1 (and changeID == -N)? It would be nice to collapse this check into if changeID > 0 and an accompanying else

changeID >= 0 and changeID == -1 both use the min/max bounds but differ in Asc vs Desc ordering.

riyazdf

ok done a full pass. Generally looking good!

riyazdf · 2017-08-28T17:40:51Z

server/storage/rethinkdb.go

+	var (
+		lower, upper, bound []interface{}
+		idx                 = "rdb_created_at_id"
+		max                 = []interface{}{gorethink.Now().Sub(blackoutTime), gorethink.MaxVal}


from IRL discussion: would be helpful to leave a comment about the compound min/max corresponding to the behavior of Between operating on an index

riyazdf · 2017-08-28T17:41:39Z

server/storage/rethinkdb.go

+}
+
+func (rdb RethinkDB) bound(changeID, filterName string) ([]interface{}, string) {
+	term := gorethink.DB(rdb.dbName).Table(Change{}.TableName()).Get(changeID).Field("created_at")


nit: rename to createdAt or similar?

riyazdf · 2017-08-28T17:45:54Z

server/storage/sql_models.go

-	ID        uint `gorm:"primary_key" sql:"not null" json:",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"`


is the added ; intended?

good catch. I initially tried some fiddling here and must have left this in by accident. It will still work as this is a valid tag format for sql but I'll remove it for consistency.

riyazdf

thanks for adding more detail to this - LGTM!

cyli

A couple of questions and a request for a test case, but this looks good otherwise.

cyli · 2017-08-28T18:51:32Z

server/storage/rethinkdb.go

+}
+
+// Change defines the the fields required for an object in the changefeed
+type Change struct {


I'm probably just missing something obvious, but since the sqldb change was renamed to SQLChange, how does the sqldb implementation of GetChanges still work, since it uses Change and not SQLChange? Is it just counting on the table names being the same and the fields being named the same (since this Changes object doesn't have any gorm tags)? (does the SQL driver automatically coerce the int ID to be a string?)

We still write using a SQLChange for the gorm side. The SQL databases -> Go will coerce fine on read, but there were some interesting issues on the insert side where "" in the ID was generating an error at the database level as it was being interpreted as an explicit NULL rather than an empty value that should be populated with an auto-increment ID.

It's interesting that even though SQLChange specifies a custom column name for GUN that it reads fine (since in our tests we check the GUN value). For safety sake, since we seem to be relying on some underlying behavior of gorm or SQL database -> go, could we also add assertions that the SHA256 value is what is expected (since that field also specifies a custom SQL column name?)

Also, would it make sense to make SQLChange unexpected, since it's an internal implementation and we are actually returning the Change struct? (And add a comment stating why?)

I can also just add the sql tags to the Change object

SQLChange and Change are both concrete types and the various storage types are implementing an interface we use at a higher level so type safety means we can never accidentally return a SQLChange

Agree that we can't accidentally return a SQLChange, I was just suggesting unexporting it and the comment for readability and future maintainership purposes.

+1 on adding the sql tags to the Change object too.

cyli · 2017-08-28T19:09:53Z

server/storage/rethinkdb.go

+// 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


Why a blackout instead of a quorum read, since we use gorethink.TableOpts{ReadMode: "majority"} for GetCurrent calls, and the change feed table is written using majority write?

This gives some additional protection in the case of a partition. Also, even with majority reads and writes, we could still end up with a little inconsistency on the most recent few records due to clock skews etc...

This gives some additional protection in the case of a partition.

Is in the goal is to make change feeds always readable so long as even one node in an n-node cluster is up?

we could still end up with a little inconsistency on the most recent few records due to clock skews etc

Could you explain how that would happen?

As I understand it, bearing in mind much of RethinkDB is a mystery to me, during a partition (I think during leader election too) we may have records being written to DBs that aren't synchronized while the partition resolves. After resolution those records end up interleaved. That means you'll get an inconsistent feed during vs after the event.

As far as clock skew, I'm using the Go server time but I don't think the rethinkdb instances are guaranteed to have perfectly synchronized clocks either. Server A inserts a record at time 12345, Server B inserts a record at 12344 according to the servers clock, but in the real world the event happens after Server A's insert. A person querying changefeed would see a record get inserted in the past, which is undesirable. The 1st priority is that the ordering of the feed is always consistent.

We don't have to worry about clock skew in the MySQL version because we use the database's time and there is only 1 replica.

I'm also pretty hazy on what RethinkDB does, especially since they don't use consensus for the data, just table metadata. But going by their consistency guarantees here: https://rethinkdb.com/docs/consistency/, we may want to specify majority reads for change feeds anyway, even with the blackout for better consistency?

According to https://aphyr.com/posts/329-jepsen-rethinkdb-2-1-5, which is a bit old but I don't think the model is outdated, only the primary replica can make changes (guaranteeing serializability of data), and the primary is selected via raft, so I'm not sure if for a single table results can be interleaved as a result of a partition (since only the primary can cause writes).

I can see what you mean about the clock skew though, although if I'm reading this correctly, we order by CreatedAt, which is determined by the notary server's clock, as opposed to the rethink server's clock? Do we perform any checks on the notary server that would guarantee that the clock skew does not exceed 60 seconds?

TL;DR version - maybe we want to add a majority read option to the GetChanges query, but other than that, the rest of the comment is just questions that don't affect the PR.

No, we're relying on NTP or some custom mechanism to keep all the notary servers reasonably in sync.

cyli · 2017-08-28T19:28:36Z

server/storage/rethinkdb.go

+// 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) {
+	createdAtTerm := gorethink.DB(rdb.dbName).Table(Change{}.TableName()).Get(changeID).Field("created_at")


What happens if the change ID doesn't exist in the table? (and can we have a test for that case?)

Good question. Adding the test case. I think this is where the behaviours may unfortunately diverge between gorm and rethinkdb. RethinkDB will likely error due to the bound function looking up an explicit change ID, while MySQL will happily just use the ID as a bound.

Ok, test pushed. As far as the API spec, it probably makes sense for us to update it to specify the behavior is "undefined" (gives us a lot of wiggle room) in the cases where a changeID other than 0, -1, or a known valid ID are provided.

I added 2 variations of the test to simulate multiple expected behaviours 51bcded#diff-523c4d973493ddd421ebe1b81b5ec81eR432

Thanks for the update! Wondering if we should catch these specifically (where the ID can't be coerced to an int in SQL, or that the ID doesn't exist in rethink, and return an InvalidChangeID type error? Otherwise I think in the handler this gets translated into a 500, whereas it should probably be a 400? Otherwise we might see some spurious reports/pages/etc.

cyli · 2017-08-28T19:30:42Z

server/storage/rethinkdb.go

+	// 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"


"open" means including the values, right?

"open" means excluding values.

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

cyli

LGTM on green, although would be awesome if we could apply ErrBadQuery to rethink as well.

endophage · 2017-08-30T17:57:33Z

@cyli filed an issue to track that #1216

endophage requested a review from jlhawn August 24, 2017 23:26

endophage force-pushed the changefeed branch 19 times, most recently from 7dab18a to 993614b Compare August 26, 2017 01:04

changefeed for rethinkdb

005f17e

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

endophage force-pushed the changefeed branch 2 times, most recently from de530db to 0f070e0 Compare August 26, 2017 03:26

defer the reversing

07a5c04

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

endophage force-pushed the changefeed branch from 0f070e0 to 07a5c04 Compare August 26, 2017 03:50

riyazdf reviewed Aug 28, 2017

View reviewed changes

riyazdf approved these changes Aug 28, 2017

View reviewed changes

cyli reviewed Aug 28, 2017

View reviewed changes

endophage force-pushed the changefeed branch 8 times, most recently from 8c36ed3 to 6aa8149 Compare August 28, 2017 21:58

review comments

55f635c

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

endophage force-pushed the changefeed branch from 6aa8149 to 55f635c Compare August 28, 2017 22:11

cyli approved these changes Aug 28, 2017

View reviewed changes

endophage merged commit ab64f58 into notaryproject:master Aug 28, 2017

endophage deleted the changefeed branch August 30, 2017 17:56

riyazdf mentioned this pull request Nov 14, 2017

Work out what rethinkdb returns that we should translate to ErrBadQuery for changefeed #1216

Open

changefeed for rethinkdb #1214

changefeed for rethinkdb #1214

Conversation

endophage commented Aug 24, 2017

riyazdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf left a comment

Choose a reason for hiding this comment

cyli left a comment

Choose a reason for hiding this comment

cyli Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endophage Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

cyli Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli left a comment

Choose a reason for hiding this comment

endophage commented Aug 30, 2017

cyli Aug 28, 2017 •

edited

Loading

cyli Aug 28, 2017 •

edited

Loading

endophage Aug 28, 2017 •

edited

Loading

cyli Aug 28, 2017 •

edited

Loading