Skip to content

fix: honor --dstNodes in balance-segment - #508

Merged
sre-ci-robot merged 1 commit into
milvus-io:mainfrom
czs007:fix/balance-segment-dstnode
Jul 27, 2026
Merged

fix: honor --dstNodes in balance-segment#508
sre-ci-robot merged 1 commit into
milvus-io:mainfrom
czs007:fix/balance-segment-dstnode

Conversation

@czs007

@czs007 czs007 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What

balance-segment accepts a destination-node flag, but the value was never sent to querycoord.

BalanceSegmentCommand parses the flag into p.DstNodes and then builds the request without it:

req := &querypb.LoadBalanceRequest{
    Base:             &commonpb.MsgBase{TargetID: s.session.ServerID},
    CollectionID:     p.CollectionID,
    SealedSegmentIDs: p.SegmentIDs,
    SourceNodeIDs:    p.SourceNodes,
}   // p.DstNodes is never used

On the server side, an empty DstNodeIDs means "any RW node of the replica" — querycoord inserts replica.GetRWNodes() into the destination set and lets the assign policy pick. So the flag is silently ignored and the caller gets a destination they did not ask for, with no error.

Repro

Against a local Milvus 2.6.19 cluster (3 querynodes: 8, 10, 11):

$ balance-segment --collection 467844892567274894 \
      --segment 467844892568376490 --srcNodes 10 --dstNode 8
(success, no error)

$ show segment-loaded-grpc
ServerID 8     (segment not here)
ServerID 10    (segment not here)
ServerID 11
SegmentID: 467844892568376490 ... NumOfRows 120000     <- landed on 11, not the requested 8

Fix

Populate DstNodeIDs from the flag.

Per review feedback, DstNodes is []int64 with flag --dstNodes, mirroring SourceNodes / --srcNodes and the repeated DstNodeIDs field in the proto. An empty value keeps the previous behaviour (let querycoord pick), so existing usage is unaffected — and nothing could have depended on the old --dstNode spelling, since the value was never sent.

Test

Rebuilt and re-ran the same command against the same cluster; the segment now lands on the requested node. go build ./... and gofmt clean.

Backport to v1.1.x: #509.

@mergify

mergify Bot commented Jul 22, 2026

Copy link
Copy Markdown

@czs007 Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

@mergify mergify Bot added the needs-dco label Jul 22, 2026
@czs007
czs007 force-pushed the fix/balance-segment-dstnode branch from 2870eab to 0e10a28 Compare July 22, 2026 18:19
@mergify mergify Bot added dco-passed and removed needs-dco labels Jul 22, 2026
@mergify

mergify Bot commented Jul 22, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Comment thread states/mgrpc/querycoord.go Outdated
SegmentIDs []int64 `name:"segment" desc:"segment ids to balance"`
SourceNodes []int64 `name:"srcNodes" desc:"from querynode ids"`
DstNodes int64 `name:"dstNode" desc:"to querynode ids"`
DstNodes int64 `name:"dstNode" default:"0" desc:"to querynode id, 0 means let querycoord pick"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use []int64 like SourceNodes for DstNodes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, done in 1d01527.

DstNodes is now []int64 with flag --dstNodes, mirroring SourceNodes / --srcNodes and the repeated DstNodeIDs field in the proto. This also lets the 0 sentinel and the if p.DstNodes != 0 branch go away — an empty slice already means "let querycoord pick".

I renamed the flag from --dstNode to --dstNodes for symmetry; nothing depended on the old spelling since the value was never sent to querycoord in the first place. Happy to keep --dstNode as the flag name if you would rather not change it.

Backport #509 is updated the same way.

`BalanceSegmentCommand` parsed the destination flag into `p.DstNodes` but
never put it into the `LoadBalanceRequest`, so the flag was silently
ignored and querycoord always fell back to picking a destination among
all nodes of the replica (`DstNodeIDs` empty means "any RW node").

Verified against a local Milvus 2.6.19 cluster: running
`balance-segment --collection C --segment S --srcNodes 10 --dstNode 8`
returned success but the segment landed on node 11, not node 8.

Populate `DstNodeIDs` from the flag. `DstNodes` is now `[]int64` /
`--dstNodes`, mirroring `SourceNodes` / `--srcNodes` and the repeated
`DstNodeIDs` field in the proto; an empty value keeps the previous
auto-select behaviour.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
@czs007
czs007 force-pushed the fix/balance-segment-dstnode branch from 0e10a28 to 1d01527 Compare July 27, 2026 06:44
@czs007 czs007 changed the title fix: honor --dstNode in balance-segment fix: honor --dstNodes in balance-segment Jul 27, 2026

@congqixia congqixia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@sre-ci-robot sre-ci-robot added the lgtm look good to me label Jul 27, 2026
@sre-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: congqixia, czs007

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot
sre-ci-robot merged commit 7cf6ee6 into milvus-io:main Jul 27, 2026
6 checks passed
sre-ci-robot pushed a commit that referenced this pull request Jul 27, 2026
Backport of #508 to `v1.1.x`.

`BalanceSegmentCommand` parsed the destination flag into `p.DstNodes`
but never put it into the `LoadBalanceRequest`, so the flag was silently
ignored and querycoord always fell back to picking a destination among
all nodes of the replica (`DstNodeIDs` empty means "any RW node").

Populate `DstNodeIDs` from the flag. Per review feedback on #508,
`DstNodes` is `[]int64` with flag `--dstNodes`, mirroring `SourceNodes`
/ `--srcNodes` and the repeated `DstNodeIDs` field in the proto; an
empty value keeps the previous auto-select behaviour.

Cherry-picked from 1d01527.

Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants