Datastore v2 #581

jakedt · 2022-05-02T17:47:27Z

Fixes #118

jakedt · 2022-05-10T22:24:06Z

I will look into resolving any E2E test problems, but I don't think it should hold up the review.

jzelinskie

🥹

jzelinskie · 2022-05-17T19:59:36Z

pkg/datastore/datastore.go

-	DeleteRelationships(ctx context.Context, preconditions []*v1.Precondition, filter *v1.RelationshipFilter) (Revision, error)
+	// ReadWriteTx tarts a read/write transaction, which will be committed if no error is
+	// returned and rolled back if an error is returned.
+	ReadWriteTx(context.Context, TxUserFunc) (Revision, error)

 	// OptimizedRevision gets a revision that will likely already be replicated


Is this comment right? I can't expand it for some reason

josephschorr · 2022-05-18T20:24:10Z

internal/datastore/common/sql.go

+// occurred when building the transaction.
+type TxFactory func(context.Context) (pgx.Tx, TxCleanupFunc, error)
+
+func NewPGXExecutor(txSource TxFactory) ExecuteQueryFunc {


Doc comment?

josephschorr · 2022-05-18T20:26:20Z

internal/datastore/crdb/crdb.go

+
+		cleanup := func(ctx context.Context) {
+			if err := tx.Rollback(ctx); err != nil {
+				log.Ctx(ctx).Err(err).Msg("error rolling back transaction")


Add additional context to the error message, since it is the same message as below, and once line numbers move, it'll be harder to pin down the exact location if we see the error occur

i mean they will have different underlying errors.

josephschorr · 2022-05-18T20:29:00Z

internal/datastore/crdb/readwrite.go

+	bulkTouch := queryTouchTuple
+	var bulkTouchCount int64
+
+	// Process the actual updates	var tupleCountChange int64


var tupleCountChange int64?

josephschorr · 2022-05-18T20:30:58Z

internal/datastore/crdb/readwrite.go

+func (rwt *crdbReadWriteTXN) DeleteRelationships(filter *v1.RelationshipFilter) error {
+	ctx, span := tracer.Start(datastore.SeparateContextWithTracing(rwt.ctx), "DeleteRelationships")
+	defer span.End()
+	// Add clauses for the ResourceFilter


newline before this line

josephschorr · 2022-05-18T20:31:30Z

internal/datastore/crdb/readwrite.go

+		if err != nil {
+			return fmt.Errorf(errUnableToWriteConfig, err)
+		}
+		query = query.Values(newConfig.Name, serialized)


We might have a risk of hitting query limits here too

Yeah, there is a hard upper bound on how big our schema can get now, but it's unlikely to hit this limit without first hitting the overall schema byte limit.

josephschorr · 2022-05-18T20:43:11Z

internal/datastore/memdb/memdb.go

+			return tx, err
+		}
+
+		newRevision := revisionFromTimestamp(time.Now().UTC())


josephschorr · 2022-05-18T20:43:38Z

internal/datastore/memdb/memdb.go

+		}
+		if tx != nil {
+			for _, change := range tx.Changes() {
+				if change.Table == "relationship" {


Place the table name into a const

josephschorr · 2022-05-18T20:45:23Z

internal/datastore/memdb/readwrite.go

+}
+
+// caller must already hold the concurrent access lock
+func (rwt *memdbReadWriteTx) delete(tx *memdb.Txn, filter *v1.RelationshipFilter) error {


Maybe name to indicate the caller must hold the lock?

josephschorr · 2022-05-18T20:52:51Z

internal/datastore/mysql/connection.go

+		return nil, fmt.Errorf("failed to open connection to db: %w", err)
+	}
+
+	// The go mysql driver implements the ExecerContext interface, assert that here.


ExecerContext?

I mean you can see it on the next line. What do you want here?

Just seems a weird name, but its fine

I didn't name it, it's part of the go sql interface.

josephschorr · 2022-05-18T20:59:36Z

internal/datastore/mysql/datastore.go

+
+			rwt := &mysqlReadWriteTXN{
+				&mysqlReader{
+					mds.QueryBuilder,


True... it just makes it harder to read, but I'd rather have the safety.

josephschorr · 2022-05-19T15:46:56Z

internal/datastore/memdb/schema.go

+	))
+}
+
+func (r relationship) Relationship() *v1.Relationship {


I think the core package provides conversions that could remove the need for this

No, this is going from the flat struct that we use to represent a row in the memdb to the relationship.

We do convert to a core rel below, so we could in theory chain that way, but not sure it is worth it

josephschorr · 2022-05-19T15:49:07Z

internal/datastore/mysql/readwrite.go

+	ctx, span := tracer.Start(ctx, "WriteNamespaces")
+	defer span.End()
+
+	// span.SetAttributes(common.ObjNamespaceNameKey.String(newNamespace.Name))


Add a new key and set it to the list of namespace names?

josephschorr

LGTM

jakedt added the area/datastore Affects the storage system label May 2, 2022

jakedt requested a review from a team as a code owner May 2, 2022 17:47

jakedt marked this pull request as draft May 2, 2022 17:47

github-actions bot added area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels May 2, 2022

jakedt force-pushed the datastore-v2 branch 2 times, most recently from 5d5ebd8 to ff3ac89 Compare May 4, 2022 19:01

github-actions bot added area/api v0 Affects the v0 API area/api v1 Affects the v1 API area/CLI Affects the command line area/dashboard Affects the dashboard area/dispatch Affects dispatching of requests labels May 4, 2022

jakedt force-pushed the datastore-v2 branch from ff3ac89 to 40c34de Compare May 4, 2022 19:05

github-actions bot removed the area/dependencies Affects dependencies label May 4, 2022

jakedt force-pushed the datastore-v2 branch from 40c34de to d36f2fd Compare May 4, 2022 22:29

This was referenced May 5, 2022

Ensure preconditions on tuple writes in Postgres and MySQL honor deletions #547

Closed

Reduce the namespace manager interface to the essentials #544

Closed

jakedt force-pushed the datastore-v2 branch 4 times, most recently from 9e5f9ac to 6cb191f Compare May 10, 2022 14:14

github-actions bot added the area/dependencies Affects dependencies label May 10, 2022

jakedt force-pushed the datastore-v2 branch 2 times, most recently from fa897aa to 4ecb13a Compare May 10, 2022 22:21

jakedt marked this pull request as ready for review May 10, 2022 22:23

jakedt requested a review from vroldanbet as a code owner May 10, 2022 22:23

jakedt requested review from ecordell and jzelinskie May 10, 2022 22:25

jakedt force-pushed the datastore-v2 branch from 4ecb13a to 7733c36 Compare May 11, 2022 18:45

jakedt force-pushed the datastore-v2 branch from 5d71014 to d5bba2f Compare May 16, 2022 22:30

jzelinskie previously approved these changes May 17, 2022

View reviewed changes

jakedt dismissed jzelinskie’s stale review via 74291f7 May 18, 2022 14:13

jakedt force-pushed the datastore-v2 branch 2 times, most recently from 74291f7 to e9b7d21 Compare May 18, 2022 14:14

josephschorr reviewed May 18, 2022

View reviewed changes

josephschorr reviewed May 19, 2022

View reviewed changes

jakedt force-pushed the datastore-v2 branch from e9b7d21 to a0afdcc Compare May 19, 2022 22:14

ecordell mentioned this pull request May 20, 2022

prevent crdb from crashing in e2e tests #611

Merged

jakedt added 15 commits May 20, 2022 13:03

datastore: add a v2 interface

4d05f90

datastore: update tests for v2 interface

a3f429d

memdb: add a datastore v2 implementation

00bdd35

datastore: v2 datastore proxy implementations

0f832c6

datastore: add a namespace caching proxy

2effd51

add preconditions checking helper

3cdb63c

Update call sites for datastore v2 and remove namespace manager.

ae4552e

datastore: remove NamespaceCacheKey method

38effbe

datastore: update common SQL code to support v2

72bf9b9

postgres: convert postgres to datastore v2 API

b00d123

crdb: convert to datastore v2 API

c636bf6

datastore: forward logger through separated context

8dd6ca3

spanner: convert to datastore v2 API

5eb8642

mysql: convert to datstore v2 API

cb53cc2

cmd: default retries to 10, applies to more datastores

56408c3

jakedt force-pushed the datastore-v2 branch from a0afdcc to 56408c3 Compare May 20, 2022 17:03

josephschorr approved these changes May 20, 2022

View reviewed changes

jakedt enabled auto-merge May 20, 2022 17:04

jakedt merged commit fc39ad4 into main May 20, 2022

jakedt deleted the datastore-v2 branch May 20, 2022 19:53

github-actions bot locked and limited conversation to collaborators May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datastore v2 #581

Datastore v2 #581

jakedt commented May 2, 2022 •

edited by ecordell

Loading

jakedt commented May 10, 2022

jzelinskie left a comment

jzelinskie May 17, 2022

josephschorr May 18, 2022

josephschorr May 18, 2022

jakedt May 19, 2022

josephschorr May 18, 2022

josephschorr May 18, 2022

josephschorr May 18, 2022

jakedt May 19, 2022 •

edited

Loading

josephschorr May 18, 2022

josephschorr May 18, 2022

josephschorr May 18, 2022

josephschorr May 18, 2022

jakedt May 19, 2022

josephschorr May 20, 2022

jakedt May 20, 2022

josephschorr May 18, 2022

josephschorr May 19, 2022

jakedt May 19, 2022

josephschorr May 20, 2022

josephschorr May 19, 2022

josephschorr left a comment

Datastore v2 #581

Datastore v2 #581

Conversation

jakedt commented May 2, 2022 • edited by ecordell Loading

jakedt commented May 10, 2022

jzelinskie 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

jakedt May 19, 2022 • 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

josephschorr left a comment

Choose a reason for hiding this comment

jakedt commented May 2, 2022 •

edited by ecordell

Loading

jakedt May 19, 2022 •

edited

Loading