Skip to content
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

sql,distsqlrun,gossip: don't plan on incompatible nodes #17747

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

andreimatei
Copy link
Contributor

If we attempt to schedule a flow on a node who's DistSQL version is
incompatible, the scheduling is going to return an error.
This patch attempts to minimize the occurrence of these errors by not
planning flows on incompatible nodes. This is done by having each node
gossip it's range of accepted versions, and having planning consult
gossip before deciding to map key spans onto a node. Spans owned by
incompatible nodes are mapped to the gateway.

The planning version check is done in distSQLPlanner.partitionSpans().
This may not sounds like the right place for it, but there's currently
no better place. That's the layer that's currently similarly concerned
with node health because everything planned above TableReaders currently
mechanically follows the set of nodes decided in partitionSpans().
An alternative would be to lift the remapping onto the gateway to a
step done after the plan has been build, but that would lead to worse
plans.

@andreimatei andreimatei requested review from a team August 17, 2017 23:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andreimatei
Copy link
Contributor Author

@bdarnell for gossip stuff
cc @RaduBerinde

I ran into trouble with #17497 which was supposed to make us resilient to DistSQL version mismatch errors (and other errors) through a runtime fallback mechanism - the fundamental issue is that it's hard to decide when to fallback because it's hard to conclude that you've connected to the wrong node.
So let's do this about version mismatch errors while I ponder fallbacks some more.

@RaduBerinde
Copy link
Member

Review status: 0 of 13 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/distsql_physical_planner.go, line 517 at r1 (raw file):

					if compat, ok = nodeVerCompatMap[nodeID]; !ok {
						compat = dsp.nodeVersionIsCompatible(nodeID, dsp.planVersion)
						nodeVerCompatMap[nodeID] = compat

If we have an older node, then we upgrade it, it will be stuck as non-compatible forever no? Ideally these entries should time out


pkg/sql/executor.go, line 339 at r1 (raw file):

	// accepting any version, the callback can return a 0..+inf acceptable version
	// range.
	OverrideDistSQLVersionCheck func(node roachpb.NodeID, res *distsqlrun.DistSQLVersionGossipInfo) error

[nit] Would be cleaner for this to return (distsqlrun.DistSQLVersionGossipInfo, error) instead of taking a pointer


Comments from Reviewable

@andreimatei
Copy link
Contributor Author

Review status: 0 of 13 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/distsql_physical_planner.go, line 517 at r1 (raw file):

Previously, RaduBerinde wrote…

If we have an older node, then we upgrade it, it will be stuck as non-compatible forever no? Ideally these entries should time out

nodeVerMap is just a per-query map (like planCtx.nodeAddresses).
When a node is upgraded, it will gossip its new version.


Comments from Reviewable

@RaduBerinde
Copy link
Member

:lgtm:


Review status: 0 of 13 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/distsql_physical_planner.go, line 517 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nodeVerMap is just a per-query map (like planCtx.nodeAddresses).
When a node is upgraded, it will gossip its new version.

Ah, indeed, sorry.


Comments from Reviewable

@andreimatei andreimatei force-pushed the distsql-gossip-version branch from 110558c to 8c859ba Compare August 21, 2017 23:24
@andreimatei
Copy link
Contributor Author

Review status: 0 of 13 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/executor.go, line 339 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Would be cleaner for this to return (distsqlrun.DistSQLVersionGossipInfo, error) instead of taking a pointer

done. I had written it like this to resemble the gossip interface, but I think I had already departed from that untyped interface.


Comments from Reviewable

@andreimatei andreimatei force-pushed the distsql-gossip-version branch from 8c859ba to 779879f Compare August 22, 2017 15:37
If we attempt to schedule a flow on a node who's DistSQL version is
incompatible, the scheduling is going to return an error.
This patch attempts to minimize the occurrence of these errors by not
planning flows on incompatible nodes. This is done by having each node
gossip it's range of accepted versions, and having planning consult
gossip before deciding to map key spans onto a node. Spans owned by
incompatible nodes are mapped to the gateway.

The planning version check is done in distSQLPlanner.partitionSpans().
This may not sounds like the right place for it, but there's currently
no better place. That's the layer that's currently similarly concerned
with node health because everything planned above TableReaders currently
mechanically follows the set of nodes decided in partitionSpans().
An alternative would be to lift the remapping onto the gateway to a
step done after the plan has been build, but that would lead to worse
plans.
@andreimatei andreimatei force-pushed the distsql-gossip-version branch from 779879f to 02ff564 Compare August 22, 2017 16:22
@andreimatei andreimatei merged commit 334f85e into cockroachdb:master Aug 22, 2017
@andreimatei andreimatei deleted the distsql-gossip-version branch August 22, 2017 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants