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

distsql: remove possibility of inbound stream timeout errors #27753

Closed
asubiotto opened this issue Jul 19, 2018 · 3 comments
Closed

distsql: remove possibility of inbound stream timeout errors #27753

asubiotto opened this issue Jul 19, 2018 · 3 comments
Labels
A-sql-execution Relating to SQL execution. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@asubiotto
Copy link
Contributor

We currently set up a flow by sending out n SetupFlowRequests to all participating nodes in a query. These then set up any necessary connections between them by having outboxes send a FlowStream RPC to a destination node. These FlowStream RPCs wait up to a timeout for the corresponding flow to be registered on the destination node. Note that a SetupFlowRequest does not actually call RegisterFlow. That only happens once Start is called.

We have run into a bunch of issues with streams timing out because these two events don't line up within the timeout. I think we could get rid of a lot of these cases by having SetupFlowRequests include the FlowSpecs of all downstream nodes. The FlowStream requests would use these and could be made responsible to set up the flows on remote nodes actively instead of waiting for a SetupFlowRequest if the flow isn't registered.

More details to come.

@asubiotto asubiotto added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-execution Relating to SQL execution. labels Jul 19, 2018
@asubiotto asubiotto added this to the 2.1 milestone Jul 19, 2018
@petermattis petermattis removed this from the 2.1 milestone Oct 5, 2018
@jordanlewis
Copy link
Member

@asubiotto is this still a thing you want to do?

@asubiotto
Copy link
Contributor Author

I think it would be interesting to explore, but don't think it's a high priority.

@asubiotto
Copy link
Contributor Author

I explored this but don't think we want to go down this road at this time. There is essentially only one FlowSetupRequest that needs to be sent to any given node, but with this approach we'll introduce a lot of redundancy (essentially copying the same FlowSetupRequest to each outbox in a flow). This issue was prompted by streams timing out under load, which is something that @ajwerner's work will probably obviate so it seems better to keep the current, simple, status quo than try going down a path without many concrete benefits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-execution Relating to SQL execution. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

3 participants