distsqlrun: schedule failed streams on the gateway

[andreimatei]

Before this patch, failure to schedule any remote flows was causing the
query to fail. This is not great, as scheduling can fail because the
remote node went down (but it must have gone down recently, otherwise we
wouldn't even have scheduled a flow on it), but also because the remote
node is running an older version.
This patch makes distSQLPlanner.Run() schedule locally the flows that
failed to schedule remotely. It does this by merging the processors in
those flows with the local flow.
Moving flows to the gateway means that any other remote processor that
was planned to connect to a moved processor will have a bad time
connecting; we fix it by adding a fallback to outboxes - all outboxes
are programmed to fallback to connecting to the gateway if the
connection to their primary target doesn't succeed in within a timeout.

[cockroach-teamcity]

This change is 

[andreimatei]

I still have to figure out the testing story, but sending out for opinions.

[RaduBerinde]

This is cool stuff! Thanks for getting this done so quickly!

For testing, we could add a testing knob to inject version mismatch errors and verify a query while turning it on and off for various nodes.

I worked on a change at some point that introduced an "finished flow cache" which would fail-fast requests for those flows, but never made it into a PR. The idea was that if a flow fails and gets unregistered before other flows connect to it, we don't want those to wait for the timeout. The same could be used instead of the poisoning stuff. If you want to look at it, this is the commit: RaduBerinde@3eb5dd0 We can of course add this later, just thought I should point it out.

---

Review status: 0 of 13 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

pkg/sql/distsql_running.go, line 41 at r3 (raw file):

// poisonedFlowDefaultTimeout is the amount of time that a poisoned flow (a
// flow that will not actually be scheduled) lives in the FlowRegistry.
const poisonedFlowDefaultTimeout time.Duration = time.Second

this can be longer? the connection timeouts are a few seconds I think

---

pkg/sql/distsqlrun/flow_registry.go, line 205 at r3 (raw file):

// late streams attempting to connect will wait for the regular connection
// timeout before timing out.
func (fr *flowRegistry) PoisonFlow(id FlowID, timeout time.Duration) {

[nit] I would call this "duration" (as in duration of the poisoning)

---

pkg/sql/distsqlrun/flow_registry.go, line 229 at r3 (raw file):

		fr.UnregisterFlow(id)
	})

[nit] extra blank line

---

pkg/sql/distsqlrun/outbox.go, line 320 at r2 (raw file):

	}
	var fallbackTimer *time.Timer
	if m.fallbackAddr != "" {

[nit] would be a bit more readable to use a hasFallback := (m.fallbackAddr != "") instead of repeating the condition

---

pkg/sql/distsqlrun/outbox.go, line 323 at r2 (raw file):

		connectAttemptsRemaning++
		fallbackStreamCtx, fallbackStreamCancel = context.WithCancel(ctx)
		fallbackTimer = time.AfterFunc(outboxAttemptFallbackTimeout, func() {

Pretty smart stuff going on here with the timer!

---

pkg/sql/distsqlrun/outbox.go, line 359 at r2 (raw file):

	if m.fallbackAddr != "" {
		return errors.Errorf(
			"failed to connect outbound stream. Primary: %s. Fallback: %s",

I think it's ok to have the same message (with empty fallback if there is none); it's a bit more explicit than the other one.

---

pkg/sql/distsqlrun/outbox.go, line 365 at r2 (raw file):

}

func connectOutboundStream(

Can this not have the same name as the method above?

---

Comments from Reviewable

[knz]

Maybe rebase this? Also it will want a release note.

[andreimatei]

This PR is dead, at least for now. 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.

…

On Fri, Apr 27, 2018 at 10:53 AM, kena ***@***.***> wrote: Maybe rebase this? Also it will want a release note. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#17497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXBcQgxFL82hW_yujocKKiiG-WrBCleks5tszDUgaJpZM4OwApb> .

[jordanlewis]

cc @asubiotto

[rjnn]

Closing this as obsolete.