Local first optimizations #450

carsonfarmer · 2020-10-08T23:15:49Z

This PR contains a few small nits and optimizations that make building on offline-first client for ThreadDB easier. The main changes are that a call to Save can now also lead to a Create action being generated if and only if the call to Save includes an existing Instance ID as a simple safety check. This allows callers to write or overwrite an existing Instance, without having to check first if it already exists. An additional change is that Delete now silently ignores missing Instances. This means a client can call delete on an Instance that might have already been deleted while it was offline or similar, without throwing an error.

Finally, the more important addition is the "special" _mod field. This is simple a "readonly" timestamp that gets added to all Instances. Any changes to this field will be ignored by the peer, and will be automatically updated on Save (and initialized on Create). This doesn't really have any effect on Delete, which is partly why the ModifiedSince experimental API was also created.

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer

Some comments for when this thing is no longer WIP.

carsonfarmer · 2020-10-09T06:07:28Z

db/collection.go

 		id, err := getInstanceID(next)
 		if err != nil {
 			return nil, err
 		}
 		key := baseKey.ChildString(t.collection.name).ChildString(id.String())
 		previous, err := t.collection.db.datastore.Get(key)
 		if err == ds.ErrNotFound {
-			return nil, errCantSaveNonExistentInstance
+			a := core.Action{


This means we can "Save" an instance even if we haven't seem it before, so long as it already has a valid _id.

This is useful for reasons I've outlined elsewhere (see #440), but additionally, it circumvents a restriction on current Hub. With that in mind, we should think through the implications of this a little bit. Seems safe on the outset.

carsonfarmer · 2020-10-09T06:08:47Z

db/collection.go

@@ -656,11 +739,11 @@ func getSchemaTypeProperties(jt *jsonschema.Type, defs jsonschema.Definitions) (
 	if jt.Ref != "" {
 		parts := strings.Split(jt.Ref, "/")
 		if len(parts) < 1 {
-			return nil, ErrInvalidCollectionSchema
+			return make(map[string]*jsonschema.Type), nil


This should still be safe, because we always check and add the _id as needed. So even if the schema doesn't call for it, all ThreadDB instances will get an _id field. This makes it easier to support "empty" schemas.

carsonfarmer · 2020-10-09T06:09:27Z

db/collection.go

@@ -689,6 +772,28 @@ func setNewInstanceID(t []byte) (core.InstanceID, []byte) {
 	return newID, patchedValue
 }

+func getModifiedTag(t []byte) (int64, error) {


Just a copy-pasta pretty much from the _id equivalent.

carsonfarmer · 2020-10-09T06:09:56Z

db/collection_test.go

@@ -20,12 +21,14 @@ const (

 type Person struct {
 	ID   core.InstanceID `json:"_id"`
+	Mod  int64           `json:"_mod"`


Added for tests, this isn't needed by end-users. They can leave this off and things will work just fine.

carsonfarmer · 2020-10-09T06:10:35Z

db/collection_test.go

@@ -959,8 +1070,83 @@ func TestSaveInstance(t *testing.T) {
 		checkErr(t, err)

 		p := util.JSONFromInstance(Person{Name: "Alice", Age: 42})
-		if err := m.Save(p); !errors.Is(err, errCantSaveNonExistentInstance) {
-			t.Fatal("shouldn't save non-existent instasnce")
+		if err := m.Save(p); err != nil {


Adjusting to allow save without prior instance.

carsonfarmer · 2020-10-09T06:12:41Z

db/db.go

@@ -60,7 +61,7 @@ var (
 func init() {
 	nameRx = regexp.MustCompile(`^[A-Za-z0-9]+(?:[-][A-Za-z0-9]+)*$`)
 	// register empty map in order to gob-encode old-format events with non-encodable time.Time which cbor decodes as map[string]interface{}
-	gob.Register(map[string]interface {}{})
+	gob.Register(map[string]interface{}{})


linter I guess?

carsonfarmer · 2020-10-09T06:13:21Z

db/db_test.go

@@ -390,7 +392,8 @@ func runListenersComplexUseCase(t *testing.T, los ...ListenOption) []Action {
 	// Collection2 Save j1
 	j1 = util.SetJSONProperty("Counter", -1, j1)
 	j1 = util.SetJSONProperty("Name", "Textile33", j1)
-	checkErr(t, c2.Save(j1))
+	err = c2.Save(j1)


These are leftover from some earlier changes. I can revert them if we want.

carsonfarmer · 2020-10-09T06:13:42Z

db/dispatcher.go

@@ -43,10 +43,14 @@ func newDispatcher(store datastore.TxnDatastore) *dispatcher {
 }

 // Store returns the internal event store.
-func (d *dispatcher) Store() datastore.Datastore {
+func (d *dispatcher) Store() datastore.TxnDatastore {


Always has been a txndatastore in practice, now I want to make this explicit.

carsonfarmer · 2020-10-09T06:13:49Z

db/dispatcher.go

 	return d.store
 }

+func (d *dispatcher) Lock() *sync.RWMutex {


carsonfarmer · 2020-10-09T06:14:30Z

db/dispatcher.go

@@ -122,7 +126,8 @@ func getKey(event core.Event) (key datastore.Key, err error) {
 	}
 	time := strconv.FormatInt(unix, 10)
 	key = dsDispatcherPrefix.ChildString(time).
-		ChildString(event.InstanceID().String()).
-		ChildString(event.Collection())
+		ChildString(event.Collection()).


Up for discussion, but it defs makes queries for specific collection types within a db easier, and a lot more efficient.

As far as I know, we aren't touching this data from anywhere else, so this shouldn't require a migration or check anywhere.

Nice, good call

carsonfarmer · 2020-10-16T19:56:14Z

db/collection.go

+}
+
+// ModifiedSince returns a list of all instances that have been modified (and/or touched) since `time`.
+func (c *Collection) ModifiedSince(time int64, opts ...TxnOption) (modified []core.InstanceID, err error) {


Note that I have not yet exposed this via any gRPC APIs/updates. Partly because I can work around having this for now using less efficient means, and partly because it should be reviewed before I bother exposing it.

Would it make sense to combine this with Find, possibly as a new kind of query / condition? Then you could combine this functionality with other queries / conditions.

Hmmm, interesting. That would be ideal (no new APIs). I'm not sure off the top of my head how this might work given we are querying the dispatcher directly... but as you suggest, some sort of additional query condition/operator might do the trick. Will look into this. In that case, I might mark that API as experimental, not expose it via gRPC for now, and explore this alternative in a follow on PR. Seem reasonable?

carsonfarmer · 2020-10-16T19:57:26Z

db/collection.go

 		id, err := getInstanceID(next)
 		if err != nil {
 			return nil, err
 		}
 		key := baseKey.ChildString(t.collection.name).ChildString(id.String())
 		previous, err := t.collection.db.datastore.Get(key)
 		if err == ds.ErrNotFound {
-			return nil, errCantSaveNonExistentInstance
+			a := core.Action{


This is useful for reasons I've outlined elsewhere (see #440), but additionally, it circumvents a restriction on current Hub. With that in mind, we should think through the implications of this a little bit. Seems safe on the outset.

sanderpick

Awesome, LGTM 💪

sanderpick · 2020-10-17T05:00:21Z

db/dispatcher.go

@@ -122,7 +126,8 @@ func getKey(event core.Event) (key datastore.Key, err error) {
 	}
 	time := strconv.FormatInt(unix, 10)
 	key = dsDispatcherPrefix.ChildString(time).
-		ChildString(event.InstanceID().String()).
-		ChildString(event.Collection())
+		ChildString(event.Collection()).


Nice, good call

sanderpick · 2020-10-17T05:02:40Z

db/collection.go

+}
+
+// ModifiedSince returns a list of all instances that have been modified (and/or touched) since `time`.
+func (c *Collection) ModifiedSince(time int64, opts ...TxnOption) (modified []core.InstanceID, err error) {


Would it make sense to combine this with Find, possibly as a new kind of query / condition? Then you could combine this functionality with other queries / conditions.

carsonfarmer added 5 commits October 2, 2020 16:32

feat: adds mod tags + other nits

c53f7a2

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

feat: save returns list of saved ids. fixes #431

416e7f7

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

feat: don't error on delete of non-existant keys

95d0cb8

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

chore: revert save changes

5db16d1

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

test: deleting no longer throws errors

cae531a

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer commented Oct 9, 2020

View reviewed changes

carsonfarmer commented Oct 16, 2020

View reviewed changes

carsonfarmer marked this pull request as ready for review October 16, 2020 20:01

carsonfarmer requested a review from sanderpick October 16, 2020 20:01

sanderpick approved these changes Oct 17, 2020

View reviewed changes

carsonfarmer merged commit 43cf56e into master Oct 19, 2020

carsonfarmer deleted the carson/local-first-optims branch October 19, 2020 17:30

carsonfarmer mentioned this pull request Oct 29, 2020

Better behavior when saving non-existent instance #462

Merged

carsonfarmer mentioned this pull request Jan 12, 2021

Allow save method to write unknown objects if they already have an _id property #440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local first optimizations #450

Local first optimizations #450

carsonfarmer commented Oct 8, 2020 •

edited

Loading

carsonfarmer left a comment

carsonfarmer Oct 9, 2020

carsonfarmer Oct 16, 2020 •

edited

Loading

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 9, 2020

carsonfarmer Oct 16, 2020

sanderpick Oct 17, 2020

carsonfarmer Oct 16, 2020

sanderpick Oct 17, 2020

carsonfarmer Oct 17, 2020

sanderpick Oct 17, 2020

carsonfarmer Oct 16, 2020 •

edited

Loading

sanderpick left a comment

sanderpick Oct 17, 2020

sanderpick Oct 17, 2020

Local first optimizations #450

Local first optimizations #450

Conversation

carsonfarmer commented Oct 8, 2020 • edited Loading

carsonfarmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonfarmer Oct 16, 2020 • 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

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

carsonfarmer Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

sanderpick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonfarmer commented Oct 8, 2020 •

edited

Loading

carsonfarmer Oct 16, 2020 •

edited

Loading

carsonfarmer Oct 16, 2020 •

edited

Loading