-
Notifications
You must be signed in to change notification settings - Fork 2
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
support cross cell reads #77
Conversation
@@ -276,6 +303,7 @@ func (p PlanetScaleEdgeDatabase) sync(ctx context.Context, tc *psdbconnect.Table | |||
TableName: s.Name, | |||
Cursor: tc, | |||
TabletType: tabletType, | |||
Cells: cells, |
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.
@Phanatic Would it be better to change how psdbconnect.SyncRequest
behaves if no Cells
are given that it defaults to all cells and not only the current vtgate cell?
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.
yeah, we have an all cells
alias in planetscale_operator_default
, I can try that to see if it'll just work for Primaries & replicas.
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.
We need to filter out cells that don't match the requested tablet type.
} | ||
|
||
for _, vttablet := range tablets { | ||
if strings.EqualFold(vttablet.Keyspace, psc.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.
@mcrauwel we need to filter out by tablet type here, you can use the psc.UseReplica
here to filter.
If we pass along all the cells here, vstream might pick a primary tablet that isn't in the right cell from the list.
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 think we need to @Phanatic, we just list all the possible cells and we request a REPLICA
tablet
The tablet_picker just request an tablet to stream from in the list of cell with the tabletType == 'replica'
but if you think it's necessary, it's fairly easy to add, before we call this function we already decided the tabletType so I can just pass that along as a param...
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.
looks great!
@mcrauwel fixed this on airbyte-source in planetscale/airbyte-source#77, porting the fix with some of my changes over the vitess connector. The basic issue is that if edge is connected to a primary's vtgate and we ask to stream from a replica, vstream will just hang because it doesn't find a suitable cell to Stream from. Instead, this PR will allow the caller to give us a cell hint which should help fix this error. This is what customers would put in their connection properties: ```properties vitess.cells=planetscale_operator_default ``` Co-authored-by: <phani> Signed-off-by: Max Englander <[email protected]>
@mcrauwel fixed this on airbyte-source in planetscale/airbyte-source#77, porting the fix with some of my changes over the vitess connector. The basic issue is that if edge is connected to a primary's vtgate and we ask to stream from a replica, vstream will just hang because it doesn't find a suitable cell to Stream from. Instead, this PR will allow the caller to give us a cell hint which should help fix this error. This is what customers would put in their connection properties: ```properties vitess.cells=planetscale_operator_default ``` Co-authored-by: Phani Raj <[email protected]> Signed-off-by: Max Englander <[email protected]>
This PR makes it possible to have cross cell traffic, previous behaviour for replica's was that whenever connected to a VtGate in a cell that only has a
PRIMARY
you'd get a time-out