distsqlrun: Ignore breaker when outbox dials node

[rohany]

Fixes #38602.

Release justification: Fixes an outstanding bug.

Release note: None

[cockroach-teamcity]

This change is 

[rohany]

Let me know if this approach makes sense, and the best way to test a change like this.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @rohany, and @tbg)

---

pkg/rpc/nodedialer/nodedialer.go, line 95 at r1 (raw file):

// DialNoBreaker ignores the breaker if there is an error dialing. This function
// should only be used when there is good reason to believe that the node is reachable.
func (n *Dialer) DialNoBreaker(

it seems like this is implemented the same as the above function apart from the breaker stuff. to keep the code DRY, would it make sense to have Dial and DialNoBreaker call out to the same helper function? the only line we would want to skip in the latter case is breaker.Fail(err) i think, so the helper function could just accept a flag for that.

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @rafiss, and @tbg)

---

pkg/rpc/nodedialer/nodedialer.go, line 95 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it seems like this is implemented the same as the above function apart from the breaker stuff. to keep the code DRY, would it make sense to have Dial and DialNoBreaker call out to the same helper function? the only line we would want to skip in the latter case is breaker.Fail(err) i think, so the helper function could just accept a flag for that.

theres some other breaker related stuff happening in unexported node.dial that we want to avoid too. However, it turned out to be cleaner there than I thought than to just handle a nil breaker.

[asubiotto]

from my perspective, might be good to add a test

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @rafiss, @rohany, and @tbg)

---

pkg/rpc/nodedialer/nodedialer.go, line 193 at r2 (raw file):

	// as a stop-gap before the reconciliation occurs.
	if breaker != nil {
		breaker.Success()

Maybe make these nil checks part of the Fail and Success methods?

---

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

			// a critical part of query execution: if this step doesn't work, the
			// receiving side might end up hanging or timing out.
			// TODO(asubiotto): We should retry a failed Dial. This rests on the

I think we can remove this TODO, if we attempt to connect to a node that was healthy at plan time and it fails even while ignoring the breaker, that's probably a good enough reason to exit.

[rohany]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @rafiss, and @tbg)

---

pkg/rpc/nodedialer/nodedialer.go, line 193 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Maybe make these nil checks part of the Fail and Success methods?

I think it is more explicit this way that we are ignoring the breaker.

---

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

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think we can remove this TODO, if we attempt to connect to a node that was healthy at plan time and it fails even while ignoring the breaker, that's probably a good enough reason to exit.

Done.

[rafiss]

lgtm too, and i agree about adding a test if you can

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @rafiss, and @tbg)

[rohany]

PTAL -- i added a test for DialNoBreaker. I was originally confused about testing this because I thought we would need to recreate the situation in the issue.

[rohany]

Jk, the test passed locally but not under stress. Let me try again.

[rohany]

Alright, thanks to some help from @ajwerner (tysm) I was able to test this.

[rafiss]

the test looks great!

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @rafiss, and @tbg)

[rohany]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[rohany]

I think this is a test flake. Retrying.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[rafiss]

The flake should be resolved by #40757

[rohany]

run it back -- some other PR's got in

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)