Fix + integration test for keyspaces_to_watch routing regression [fixes #7882]
#7873
Conversation
setassociative
requested review from
deepthi,
harshit-gangal and
systay
as code owners
setassociative
force-pushed
the
vtgate_keyspaces_to_watch
branch
2 times, most recently
from
5facaae to
ad07e66
Compare
This checks if a vtgate is currently filtering keyspaces before requesting the TopoServer. This is necessary because a TopoServer can't be accessed in those cases as the filtered Topo in those cases could make it unsafe to make writes since all reads would be returning a subset of the actual topo data. The only use of the requested topoServer that I found was in the DDL handling path and was introduced in vitessio#6547. This is deployed on dev but should get testing (endtoend or unit, unclear on best path atm) before going upstream. # Conflicts: # go/vt/vtgate/vcursor_impl.go Signed-off-by: Richard Bailey <[email protected]>
Signed-off-by: Richard Bailey <[email protected]>
|
We should get this into 10.0 branch so that it can go into the release. Can you create an issue that documents the error message you were getting? It will be useful for future reference. |
Yea, I think Slack may be the only org utilizing that flag bc of how we deploy gates/tablets.
Sure thing, will do tomorrow. |
setassociative
changed the title
keyspaces_to_watch routing regressionkeyspaces_to_watch routing regression [fixes #7882]
|
Filed #7882 |
edit made in the GH web ui soooo, let's see if this doesn't break things Signed-off-by: Richard Bailey <[email protected]>
Evidently I program in the web ui now Signed-off-by: Richard <[email protected]>
| @@ -588,6 +592,9 @@ func (vc *vcursorImpl) TabletType() topodatapb.TabletType { | |||
|
|
|||
| // SubmitOnlineDDL implements the VCursor interface | |||
| func (vc *vcursorImpl) SubmitOnlineDDL(onlineDDl *schema.OnlineDDL) error { | |||
| if vc.topoServer == nil { | |||
| return vterrors.New(vtrpcpb.Code_INTERNAL, "Unable to apply DDL toposerver unavailable, ensure this vtgate is not using filtered keyspaces") | |||
There was a problem hiding this comment.
nit: phrasing.
"Unable to apply DDL because toposerver is read-only..." might be better?
|
Note: this means that online DDL cannot be used along with keyspace filtering. |
Reference: #7785, #6926 (comment) |
|
I have merged this as-is so that we can get it into 10.0. The error message can be fixed in another PR. |
Description
When we started working on online DDL support as a first-party action we introduced a dependency in the vtgate to be able to modify the topology. This conflicts with limitations enforced by a KeyspaceFilteringServer which has a access to only a subset of the topo.
As a protective measure when attempting to gain access to the Write-enabled topo.Server a filtering server throws an error. Since this happens as part of the vcursor setup to support schema changes all vtgate routing was failing if
-keyspaces_to_watchwas passed to the vtgate.I fixed this, added a new endtoend integration test to prevent future regressions, and updated how we deal with the flag value to more closely match an approach @ajm188 and I have schemed up to get flags into a more manageable state.
Unit vs integration
This is done as an integration test because adding it as a unit test relies on a fixed assumption around how keyspaces_to_watch / keyspace filtering works which I didn't want to introduce into the vcursor test suite. It also is somewhat fragile as changing the type of topo.Server used in the test would make the test invalid.
In the end all we really care about is that vtgates can route queries and so it seemed most reasonable to just make it an integration test.
Related Issue(s)
n/a
Checklist
Nominally this should be backported to 8.0-10.0 but I believe most users aren't in a position to need access to this flag so it's buyer's choice here.
Deployment Notes
This is a non-breaking change for folks who were successfully using vtgates >= 8.0.
Impacted Areas in Vitess
Components that this PR will affect: