Memdb datastore MVCC improvements #319
Conversation
github-actions
bot
added
area/api v1
jakedt
force-pushed
the
memdb-mvcc
branch
2 times, most recently
from
00f9e82
to
95b4228
Compare
| switch op { | ||
| case v0.RelationTupleUpdate_TOUCH: | ||
| // If there was a delete for the same tuple at the same revision, drop it | ||
| delete(revisionChanges.tupleDeletes, tplKey) |
There was a problem hiding this comment.
Why the difference between if I have a delete and then touch, we're left with just the touch, but if I have touch and then delete, both will be emitted?
There was a problem hiding this comment.
The won't, the test cases confirm this.
internal/datastore/common/changes.go
Outdated
|
|
||
| func (ch Changes) RevisionChanges() (changes []*datastore.RevisionChanges) { | ||
| revisionsWithChanges := make([]uint64, 0, len(ch)) | ||
| for k := range ch { |
internal/datastore/common/changes.go
Outdated
|
|
||
| for _, rev := range revisionsWithChanges { | ||
| revisionChange := &datastore.RevisionChanges{ | ||
| Revision: revisionFromTransaction(rev), |
internal/datastore/common/changes.go
Outdated
| return revisionsWithChanges[i] < revisionsWithChanges[j] | ||
| }) | ||
|
|
||
| for _, rev := range revisionsWithChanges { |
internal/datastore/common/changes.go
Outdated
| // AddChange adds a specific change to the complete list of tracked changes | ||
| func (ch Changes) AddChange( | ||
| ctx context.Context, | ||
| revision uint64, |
There was a problem hiding this comment.
revision -> revTxId or have it take in the actual revision type?
internal/datastore/postgres/watch.go
Outdated
| @@ -116,7 +114,7 @@ func (pgd *pgDatastore) loadChanges( | |||
| return | |||
| } | |||
|
|
|||
| stagedChanges := make(map[uint64]*changeRecord) | |||
| stagedChanges := make(common.Changes) | |||
There was a problem hiding this comment.
Since this is non-obvious, maybe a common.NewChanges() that does the make? (especially since if we do decide to change the type to a struct down the road, the make wouldn't work)
internal/datastore/postgres/watch.go
Outdated
| } | ||
| changes = append(changes, revisionChange) | ||
| } | ||
| changes = stagedChanges.RevisionChanges() |
internal/services/v1/watch_test.go
Outdated
| return | ||
| } | ||
|
|
||
| require.Equal(len(tc.expectedUpdates), len(receivedUpdates)) | ||
|
|
||
| for len(receivedUpdates) < len(tc.expectedUpdates) { | ||
| select { | ||
| case updates := <-updatesChan: | ||
| receivedUpdates = append(receivedUpdates, updates...) | ||
| fmt.Printf("expected: %v\n", tc.expectedUpdates) |
| require.FailNow("timed out waiting for updates") | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
We should check the updates received match, content wise, those expected
There was a problem hiding this comment.
There is no canonical ordering for the v1.RelationshipUpdates, so testing this becomes a major effort.
There was a problem hiding this comment.
Can we sort and see that we've at least seen the expected ones?
There was a problem hiding this comment.
Yes of course it's possible, but it raises the question: why was the test accepted in the first place only checking len?
There was a problem hiding this comment.
Probably an oversight in my review
| } | ||
|
|
||
| type tupleEntry struct { | ||
| type relationship struct { |
There was a problem hiding this comment.
Since this is being renamed to relationship, thoughts on renaming the members to resource/subject?
|
Updated |
internal/datastore/common/changes.go
Outdated
| } | ||
| } | ||
|
|
||
| // AsRevisionChanges returns the list of changes processes so far as a datastore watch |
internal/datastore/common/changes.go
Outdated
| } | ||
|
|
||
| // AsRevisionChanges returns the list of changes processes so far as a datastore watch | ||
| // compatible, ordered changelist. |
|
Fixes #270 |
No description provided.