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.

Sign up for GitHub

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

Jump to bottom

distsqlrun: ignore breaker when outbox dials node #38602

Closed
tbg opened this issue Jul 2, 2019 · 3 comments · Fixed by #40691
Closed

distsqlrun: ignore breaker when outbox dials node #38602

tbg opened this issue Jul 2, 2019 · 3 comments · Fixed by #40691
Assignees
@rafiss @rohany
Labels
A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.

Comments

@tbg
Copy link
Member

tbg commented Jul 2, 2019 •
edited
Loading

cockroach/pkg/sql/distsqlrun/outbox.go

Line 220 in 5a0ae02

conn, err = m.flowCtx.nodeDialer.Dial(ctx, m.nodeID)

When nodes in the cluster have recently restarted, the above dial can fail which results in the "dialee" failing with a "no inbound stream connection" error after a long timeout (10s).

In the outbox, one is relatively sure that the node is reachable (otherwise, the gateway probably would not have set the flow up that way), so it might be better in general to ignore the breaker, or to mitigate this somewhat.

See #38484 (comment)
@tbg tbg added the A-sql-execution Relating to SQL execution. label Jul 2, 2019
@tbg tbg mentioned this issue Jul 2, 2019
Closed
@awoods187 awoods187 added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting. labels Jul 23, 2019
@ajwerner ajwerner mentioned this issue Jul 30, 2019
Merged
@jordanlewis
Copy link
Member

This slipped through the cracks - would be nice to get a fix for this out in the next release.

@rafiss could you take a look at this?

@yuzefovich yuzefovich reopened this Aug 16, 2019
@yuzefovich
Copy link
Member

Oopsie, accidentally closed the issue when looking at our project board.

@knz
Copy link
Contributor

knz commented Sep 5, 2019

I just ran into this during load testing with tpcc

rohany pushed a commit to rohany/cockroach that referenced this issue Sep 11, 2019
distsqlrun: Ignore breaker when outbox dials node
f354516 
Fixes cockroachdb#38602.

Release note: None
@rohany rohany mentioned this issue Sep 11, 2019
Merged
rohany pushed a commit to rohany/cockroach that referenced this issue Sep 11, 2019
distsqlrun: Ignore breaker when outbox dials node
888452c 
Fixes cockroachdb#38602.

Release note: None
craig bot pushed a commit that referenced this issue Sep 13, 2019
Merge #40691
77a2b8a 
40691: distsqlrun: Ignore breaker when outbox dials node r=rohany a=rohany

Fixes #38602.

Release justification: Fixes an outstanding bug.

Release note: None

Co-authored-by: Rohan Yadav <rohany@cockroachlabs.com>
@craig craig bot closed this as completed in 66683d5 Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rafiss rafiss

@rohany rohany

Labels
A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

distsqlrun: Ignore breaker when outbox dials node rohany/cockroach
7 participants
@jordanlewis @knz @rafiss @tbg @yuzefovich @rohany @awoods187