compile: allow local notify streams for shuffle dispatch (#24159)#24854
Merged
mergify[bot] merged 3 commits intoJun 7, 2026
Merged
Conversation
…n#24159) This fixes the self-connection bug in the compile/remoterun notification path. `Scope.RemoteRun` already handles the local-scope case with `ipAddrMatch`, but `Scope.sendNotifyMessage` still creates a pipeline RPC stream for every `RemoteReceivRegInfo.FromAddr`. When one dispatch operator is scheduled on the same CN as the coordinator, `fromAddr` equals the local service address. The old `pipelineClient.NewStream` rejected that loopback connection with `remote run pipeline in local`, so the prepare-done notification path could not be established for the local dispatch operator. That can break remote dispatch/shuffle coordination for queries such as secondary-index UPDATE plans using shuffle join with `serial_full(...)` and surface as failure or hang. This PR keeps the existing `RemoteRun` local-execution guard intact, but allows the notification stream itself to connect to the local CN address. That is the smallest fix that restores the existing dispatch notification protocol without refactoring local/remote receiver handling. Also included: - a unit test that pins the real regression point in `pipelineClient.NewStream` Approved by: @XuPeng-SH
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Pull request overview
This PR fixes a self-connection failure in the compile/remote-run notification path by allowing pipeline RPC streams to connect to the local CN address (loopback), which is needed when a shuffle dispatch operator is scheduled on the same CN as the coordinator.
Changes:
- Removed the “reject local backend” guard in
pipelineClient.NewStream, allowing local CN notify streams to be established. - Added a unit test intended to pin the regression behavior around
pipelineClient.NewStreamaccepting local backends.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cnservice/cnclient/client.go | Removes the local-backend rejection in pipelineClient.NewStream. |
| pkg/cnservice/cnclient/client_test.go | Adds a regression unit test to ensure local backends are allowed for streams. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
XuPeng-SH
approved these changes
Jun 7, 2026
Contributor
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
LGTM. Allowing local notify streams here matches the existing higher-level guard in Scope.RemoteRun, fixes the self-connection regression in the prepare-done notify path, and keeps the change narrowly scoped. The added cnclient regression test is also a good pin for the underlying behavior.
Contributor
Merge Queue Status
This pull request spent 15 seconds in the queue, including 3 seconds running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24158
What this PR does / why we need it:
This fixes the self-connection bug in the compile/remoterun notification path.
Scope.RemoteRunalready handles the local-scope case withipAddrMatch, butScope.sendNotifyMessagestill creates a pipeline RPC stream for everyRemoteReceivRegInfo.FromAddr. When one dispatch operator is scheduled on the same CN as the coordinator,fromAddrequals the local service address. The oldpipelineClient.NewStreamrejected that loopback connection withremote run pipeline in local, so the prepare-done notification path could not be established for the local dispatch operator.That can break remote dispatch/shuffle coordination for queries such as secondary-index UPDATE plans using shuffle join with
serial_full(...)and surface as failure or hang.This PR keeps the existing
RemoteRunlocal-execution guard intact, but allows the notification stream itself to connect to the local CN address. That is the smallest fix that restores the existing dispatch notification protocol without refactoring local/remote receiver handling.Also included:
pipelineClient.NewStream