-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix performance for large schema writes in V1Alpha1 #837
Fix performance for large schema writes in V1Alpha1 #837
Conversation
8ee0c08
to
5b1ba4e
Compare
This will hopefully reduce the flakiness of the E2E test suite, which writes large batches of definitions via the V1Alpha1 WriteSchema call
5b1ba4e
to
c331676
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left 2 nit suggestions, and a potential follow up to how we handle overlapping keys in CRDB
) ([]*core.NamespaceDefinition, error) { | ||
read, err := vsr.delegate.LookupNamespaces(ctx, nsNames) | ||
if err != nil { | ||
return read, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: may confuse the reader, even though read
should be nil
return read, err | |
return nil, err |
} | ||
} | ||
|
||
return read, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return read, err | |
return read, nil |
for _, nsDef := range nsDefs { | ||
cr.addOverlapKey(nsDef.Name) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for very large namespaces (presumably the ones causing the flakiness we are attempting to fix in this PR) this would lead to many "touches" in the transactions table:
spicedb/internal/datastore/crdb/crdb.go
Lines 260 to 264 in ffdf521
for k := range rwt.overlapKeySet { | |
if _, err := tx.Exec(ctx, queryTouchTransaction, k); err != nil { | |
return fmt.Errorf("error writing overlapping keys: %w", err) | |
} | |
} |
@ecordell is there are reason we don't batch all those query operations instead of looping over the keyset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real reason other than almost all requests should have only 1 overlap key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I think it would be relatively easy to turn that into a batch pgx request
ugh sorry I didn't notice auto-merged was enabled 😭 |
This will hopefully reduce the flakiness of the E2E test suite, which writes large batches of definitions via the V1Alpha1 WriteSchema call