-
Notifications
You must be signed in to change notification settings - Fork 268
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
Read replica support for Postgres and MySQL datastores #1878
Conversation
1d5612b
to
aa51f2a
Compare
Just to clarify, this update will allow spice to use multiple reader nodes for postgres and mysql? So if I have 3 readers I would be able to pass in the entries for all of them to be used? |
@benny-yamagata Yes, the replica URI parameter is a list of URIs and the system will round robin between them |
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.
The implementation works with the assumption individual hosts will be listed, but folks most of the time would use replicas behind a load-balancer in order to be able to scale read traffic when needed. I don't think the implementation will work for that more common use case because the datastore snapshot reader does not use a single transaction.
index uint32, | ||
options ...Option, | ||
) (datastore.ReadOnlyDatastore, error) { | ||
ds, err := newMySQLDatastore(ctx, url, int(index), options...) |
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.
I searched online if there is any URI query parameter we can add to enforce read only, but didn't find any. Users should make sure the credentials provided for the replica have read-only permissions.
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.
Not really necessary; the datastore itself ensures it is read only
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.
does it install the read only datastore proxy on the replica?
return rr.chosenReader.LookupNamespacesWithNames(ctx, nsNames) | ||
} | ||
|
||
func (rr *replicatedReader) chooseSource(ctx context.Context) error { |
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.
I think returning the chosen reader in chooseSource
would better encapsulate this logic.
It uses a single connection, which means it should stay connected to the same replica |
I possibly just missed it, but it looked like you were using a |
Yeah, I traced it and it does use the pool. We'll have to do something else |
aa51f2a
to
de54d54
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.
👍🏻 on the overall strategy, this should work. My main concerns are:
- exposing
strict
via the CLI, I don't see a reason we should do that - cached checked revision proxy may race
- missing tests on
strictReaderQueryFuncs
, clarification howin progress
surfaces in a replica - clarification on Watch API behaviour, do we have a test? I understand it requires no extra work, it would just lag behind the primary
- inconsistent refactor of common datastore flags
var legacyConnPool ConnPoolConfig | ||
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-conn", DefaultReadConnPool(), &legacyConnPool) | ||
deprecateUnifiedConnFlags(flagSet) | ||
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-conn-pool-read", &legacyConnPool, &opts.ReadConnPool) | ||
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-conn-pool-write", DefaultWriteConnPool(), &opts.WriteConnPool) | ||
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-read-replica-conn-pool", DefaultReadConnPool(), &opts.ReadReplicaConnPool) |
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.
Naming pattern differs from existing conn pools. Should be datastore-conn-pool-read-replica
.
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.
I wanted all the read replica flags to start with datastore-read-replica
to be explicit
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.
I can tell you that as someone usually tinkering with these flags, having to remember 2 different naming patterns is not going to make my life easier
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.
Its a tradeoff, but I still think having all the read replica flags sharing a common prefix makes more sense. Perhaps we should ask?
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.
what is the tradeoff?
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.
When would you ever want to configure read replica pool to be not the same as the read pool?
Always: you can't use the read pool with replicas
Do we think we could use a heuristic inside SpiceDB to come up with these values rather than even exposing them at all?
Incredibly unlikely... We need to know that a replica is a replica
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.
Always: you can't use the read pool with replicas
Configure them differently, not use the same pool.
For example, if I'm using a RDS with a read replica, the read replica is likely the same size as my primary likely with the same max connections.
Incredibly unlikely... We need to know that a replica is a replica
I'm not advocating altering the system to be unaware of read replicas. I'm advocating for the system being smart enough to discover the right size of connection pools because users are very likely to pick non-ideal values themselves
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 example, if I'm using a RDS with a read replica, the read replica is likely the same size as my primary likely with the same max connections.
Which still means the read pool for non-replicas will want to be configured differently, as we also have a write pool for the primary.
I'm advocating for the system being smart enough to discover the right size of connection pools because users are very likely to pick non-ideal values themselves
And... how would it do that?
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.
Do you think a simplified version of adaptive concurrency limits (e.g. similar to tcp congestion control) would work? Basically use connection errors to adaptively discover the right number of connections for you pool -- something like this could even make the system robust to changes in the replicas or the size of the database.
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.
Perhaps, but it is IMO outside of the scope of this change (as it would apply to all pools, not just replicas), and would require a significant amount of testing.
pkg/cmd/datastore/datastore.go
Outdated
mysql.RevisionQuantization(opts.RevisionQuantization), | ||
mysql.MaxRevisionStalenessPercent(opts.MaxRevisionStalenessPercent), | ||
mysql.TablePrefix(opts.TablePrefix), | ||
mysql.WatchBufferLength(opts.WatchBufferLength), | ||
mysql.WatchBufferWriteTimeout(opts.WatchBufferWriteTimeout), | ||
mysql.WithEnablePrometheusStats(opts.EnableDatastoreMetrics), | ||
mysql.MaxRetries(uint8(opts.MaxRetries)), | ||
mysql.OverrideLockWaitTimeout(1), | ||
mysql.CredentialsProviderName(opts.ReadReplicaCredentialsProviderName), |
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.
I don't see it done for MySQL
jschorr edit since I can't reply: this is done
de54d54
to
5a5dd26
Compare
Updated |
5a5dd26
to
3d4516e
Compare
3d4516e
to
da79df9
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.
Just a few more remarks but seems ready to go
replica datastore.ReadOnlyDatastore | ||
primary datastore.Datastore | ||
|
||
chosePrimary bool |
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.
chosePrimary bool | |
chosePrimaryInTest bool |
This is accomplished by adding a datastore proxy that selects a read replica for each SnapshotReader, in a round robin fashion, and ensures that the revision being requested is available on the replica. This extra check does add some latency to the overall operation, but it should provide for the safest approach for using read replicas Fixes authzed#1321 Fixes authzed#1320
…d of using read replicas
da79df9
to
d6873f8
Compare
Updated |
d6873f8
to
c0bc83e
Compare
This will allow replicas behind load balancers to be supported (just in Postgres for now)
c0bc83e
to
0656701
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. Great work! 🚀
No description provided.