Skip to content

Support VStream with keyspaces_to_watch#8988

Merged
deepthi merged 9 commits intovitessio:mainfrom
planetscale:Keyspaces2WatchVStream
Oct 27, 2021
Merged

Support VStream with keyspaces_to_watch#8988
deepthi merged 9 commits intovitessio:mainfrom
planetscale:Keyspaces2WatchVStream

Conversation

@mattlord
Copy link
Copy Markdown
Member

@mattlord mattlord commented Oct 13, 2021 •
edited
Loading

Description

In order to safely support the VStream api against VTGates running with the -keyspaces_to_watch flag we needed to add a read-only VTTopo server interface to ensure that no changes are made to the topology from the (potentially) explicitly limited view of the topology provided via this vtgate.

Related Issue(s)

Checklist

  • Should this PR be backported? No. IMO this should bake in 13.0-snapshot
  • Tests were added
  • Documentation is not required (we never documented that these two features did NOT work together)

@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from 293a4dc to 6e04f20 Compare October 13, 2021 06:08
@mattlord mattlord requested a review from deepthi October 13, 2021 06:09
@mattlord mattlord added Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VReplication labels Oct 13, 2021
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from 6e04f20 to f1d68bd Compare October 13, 2021 06:13
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch 2 times, most recently from db8bce1 to 78532c7 Compare October 13, 2021 07:07
@mattlord
Copy link
Copy Markdown
Member Author

mattlord commented Oct 13, 2021 •
edited
Loading

The new vstream_with_keyspaces_to_watch test is failing as expected: https://github.com/vitessio/vitess/pull/8988/checks?check_run_id=3879443572

...
I1013 07:14:19.558124   18763 vstream_test.go:125] 2021-10-13 07:14:19.558123471 +0000 UTC m=+39.257232583:: remote error: Code: UNKNOWN
173
rpc error: code = Unknown desc = cannot access underlying topology server when keyspace filtering is enabled
...

@mattlord mattlord changed the title Support VStream with keyspaces_to_watch Set Support VStream with keyspaces_to_watch Oct 13, 2021
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch 18 times, most recently from b81a55e to fa7b418 Compare October 18, 2021 22:52
@mattlord mattlord marked this pull request as ready for review October 18, 2021 23:59
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from cc4a7c4 to a4b169b Compare October 22, 2021 14:38
@mattlord mattlord removed the request for review from harshit-gangal October 22, 2021 14:40
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from a4b169b to d79d53a Compare October 22, 2021 14:46
@mattlord
Add another guard rail in ReadOnlyServer topo provider
be67f3b 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch 3 times, most recently from ee1bae5 to ad6c19e Compare October 22, 2021 20:48
@mattlord
Implement Server interface in ReadOnlyServer and prevent direct acces…
4c23cf0 
…s to underlying topo.Server

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from ad6c19e to 4c23cf0 Compare October 23, 2021 04:42
sougou
sougou reviewed Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important that the we don't have dummy implementations of SetReadOnly in the servers. This can become a source of confusion in the future if someone accidentally uses one of these connections as an interface, and the implementation doesn't enforce it.

If explicit casting or the interface approach is harder, I'd recommend changing SetReadOnly to return an error, and we can have these underlying implementations fail with a "not supported" error.

@mattlord
Copy link
Copy Markdown
Member Author

mattlord commented Oct 25, 2021

I think it's important that the we don't have dummy implementations of SetReadOnly in the servers. This can become a source of confusion in the future if someone accidentally uses one of these connections as an interface, and the implementation doesn't enforce it.

If explicit casting or the interface approach is harder, I'd recommend changing SetReadOnly to return an error, and we can have these underlying implementations fail with a "not supported" error.

Good points. I'll look into this next. Thanks!

@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from 025ede4 to 258f834 Compare October 25, 2021 19:39
@mattlord
Copy link
Copy Markdown
Member Author

mattlord commented Oct 25, 2021

@sougou done: https://github.com/vitessio/vitess/pull/8988/files 🙇

@mattlord mattlord requested a review from sougou October 25, 2021 19:42
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch 6 times, most recently from d22f2aa to 838d436 Compare October 25, 2021 21:03
@mattlord
Remove IsReadOnly and SetReadOnly from Conn interface
00ec04d 
Instead handle this entirely within the generic topo Server implementation

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord force-pushed the Keyspaces2WatchVStream branch from 838d436 to 00ec04d Compare October 25, 2021 21:14
@mattlord mattlord removed the request for review from rafael October 26, 2021 23:08
sougou
sougou approved these changes Oct 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

deepthi
deepthi approved these changes Oct 27, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too. Nice work!

@deepthi deepthi merged commit d080992 into vitessio:main Oct 27, 2021
@deepthi deepthi deleted the Keyspaces2WatchVStream branch October 27, 2021 21:31
@mattlord mattlord mentioned this pull request Oct 27, 2021
Closed
@mattlord mattlord mentioned this pull request Dec 13, 2021
Merged
3 tasks
@mattlord mattlord mentioned this pull request Jan 17, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@5antelope 5antelope Awaiting requested review from 5antelope

2 more reviewers

@deepthi deepthi deepthi approved these changes

@sougou sougou sougou approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: VReplication Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mattlord @deepthi @sougou @5antelope