mma: make external caller mostly use an ExternalRangeChange #158153

sumeerbhola · 2025-11-20T18:53:19Z

An ExternalRangeChange does not have the currently mutable fields of PendingRangeChange (and the set of mutable fields is going to expand in the near future). Todos related to limitation of viewing a change as a "change replicas" or "lease transfer" etc., are moved to ExternalRangeChange, so compartmentalized to methods that are called by integration code outside the mma package.

There is still some potential future cleanup involving unexporting PendingRangeChange, which is not in scope for this PR.

The PendingRangeChange becomes a temporary container for a slice of []*pendingReplicaChanges. Printing this struct no longer involves viewing it as a "change replicas" or "lease transfer". Furthermore, the ReplicaChange.replicaChangeType field is removed and the change type is now a function of prev and next. This sets us up for making prev mutable in a future PR.

Informs #157049

Epic: CRDB-55052

Release note: None

cockroach-teamcity · 2025-11-20T18:53:31Z

This change is

sumeerbhola

This will precede #158024

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

sumeerbhola

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

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 480 at r1 (raw file):

// load. There is possibly a better way to structure this, by including the
// LoadVector and SecondaryLoadVector in the ExternalRangeChange type, and
// unexporting PendingRangeChange.

I am leaning more and more towards doing this (but not in this PR). It would happen after this PR and #158024 merge, and should mostly be mechanical (barring some unexpected difficulties).
So if you have opinions, please share.

tbg

@tbg reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @wenyihu6)

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 480 at r1 (raw file):

Previously, sumeerbhola wrote…

I am leaning more and more towards doing this (but not in this PR). It would happen after this PR and #158024 merge, and should mostly be mechanical (barring some unexpected difficulties).
So if you have opinions, please share.

I see. An example of what's mentioned in the comment is here:

cockroach/pkg/kv/kvserver/mmaintegration/allocator_sync.go

Lines 156 to 159 in 0988115

    
           if kvserverbase.LoadBasedRebalancingModeIsMMA(&as.st.SV) { 
        
           	change := convertLeaseTransferToMMA(desc, usage, transferFrom, transferTo) 
        
           	mmaChange, isMMARegistered = as.mmaAllocator.RegisterExternalChange(change) 
        
           }

package mmaintegration is making a PendingRangeChange that represents the load.
And you're proposing that the alternative would be to lift the load vectors directly onto the ExternalRangeChange type.

That makes sense to me. It does get a bit confusing when there is an ExternalRangeChange and PendingRangeChange and code that creates an external one needs to handle both.

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 104 at r1 (raw file):

// IsChangeReplicas returns true if the range change is a change replicas
// operation.
func (rc *ExternalRangeChange) IsChangeReplicas() bool {

The bulk of this file would've been easier to review if you had changed the receiver in-place and then moved in a separate commit (or the other way around). As is, I'm looking at 200+ lines disappearing in one file and 200+ lines appearing in this file. I assume they are the same lines (mod mechanical changes) but that shouldn't be something I have to assume or check manually. Please consider that for next time.
For this time around: is this a mechanical movement / receiver name or are there bits that should be reviewed closely?

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 586 at r1 (raw file):

// isTransferLease returns true if the pending range change is a transfer
// lease operation.

isPureTransferLease would be even clearer as a name. Every time I see anything about lease transfers, my mind has to rattle on whether we're talking "pure" or also wrapped lease transfers.

I'm also noting that we have similar-looking methods on PendingRangeChange and ExternalRangeChange, which is a bit sad but not sure it can be changed. Either way, if we update name or comment here we should do it there too.

Suggestion:

// isTransferLease returns true if the pending range change is purely a transfer
// lease operation, in particular not a combined replication change and lease transfer.

tbg · 2025-11-21T14:10:33Z

Looks good to me. I'll leave to @wenyihu6 to approve since I think she was also planning on reviewing this.

sumeerbhola

TFTR!

@sumeerbhola reviewed 1 of 12 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg and @wenyihu6)

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 480 at r1 (raw file):

It does get a bit confusing when there is an ExternalRangeChange and PendingRangeChange and code that creates an external one needs to handle both.

Exactly. But I also don't want to duplicate code in doing that cleanup, so I will need to try it out and see how well it works. Hence the sequencing after the more important pending changes cleanup.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 586 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

isPureTransferLease would be even clearer as a name. Every time I see anything about lease transfers, my mind has to rattle on whether we're talking "pure" or also wrapped lease transfers.

I'm also noting that we have similar-looking methods on PendingRangeChange and ExternalRangeChange, which is a bit sad but not sure it can be changed. Either way, if we update name or comment here we should do it there too.

Done

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 104 at r1 (raw file):

The bulk of this file would've been easier to review if you had changed the receiver in-place and then moved in a separate commit (or the other way around)

It is a mechanical move with fixing up field names. Apologies -- it totally slipped my mind to first move the methods of PendingRangeChange here.

sumeerbhola

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

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 104 at r1 (raw file):

Previously, sumeerbhola wrote…

The bulk of this file would've been easier to review if you had changed the receiver in-place and then moved in a separate commit (or the other way around)

It is a mechanical move with fixing up field names. Apologies -- it totally slipped my mind to first move the methods of PendingRangeChange here.

I've added a first commit that does the move. This should be easy now.

This is in preparation for them becoming methods on ExternalRangeChange.

tbg

Thanks!

@tbg reviewed 11 of 11 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @wenyihu6)

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 480 at r1 (raw file):

Previously, sumeerbhola wrote…

It does get a bit confusing when there is an ExternalRangeChange and PendingRangeChange and code that creates an external one needs to handle both.

Exactly. But I also don't want to duplicate code in doing that cleanup, so I will need to try it out and see how well it works. Hence the sequencing after the more important pending changes cleanup.

I'm okay with that sequencing.

wenyihu6

Sorry for the late review here.

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

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 605 at r3 (raw file):

			foundAddLease = true
		} else {
			return false

What does reaching here mean? I assume this is in future PRs, but perhaps we can de-duplicate this logic which is already in ExternalRangeChange.IsTransferLease if it is easy to clean up.

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 38 at r3 (raw file):

	// ReplicaChange.next, which is shared by this field.
	Next ReplicaIDAndType
	// ChangeType is a function of (Prev, Next) and a convenience.

In the next PR, I’d appreciate a comment explaining that ExternalReplicaChange’s fields are immutable, which is why ChangeType can be static here. It would also be helpful to note how these fields can diverge from PendingRangeChange over time, and that mma cannot use ExternalReplicaChange internally in a meaningful way.

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 247 at r3 (raw file):

// isOnlyLeaseChange returns true if the change is only a change to the
// leaseholder status.
func (c ExternalReplicaChange) isOnlyLeaseChange() bool {

nit: can we align the names isOnlyLeaseChange and IsPureTransferLease? I think one is on ExternalReplicaChange and another is on PendingRangeChange, but they represent the same concept is that right

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 254 at r1 (raw file):

}

func (rc ReplicaChange) replicaChangeType() ReplicaChangeType {

For next PR, a comment on how this might change and that is why we have to keep callling replicaChangeType could be useful.

wenyihu6 · 2025-11-21T16:08:55Z

I think the PR changed when I was reviewing - to confirm, was this just a rebase https://github.com/cockroachdb/cockroach/compare/697d317ae101ad504ca8a9e0cf701a0715f7b0c4..2c19ef22d9b58391cc46f5e88c74bb10bf9505c4?

sumeerbhola · 2025-11-21T16:17:54Z

I think the PR changed when I was reviewing - to confirm, was this just a rebase

Yes, I was picking up the fix related to https://cockroachlabs.slack.com/archives/C048HDZJSAY/p1763736402408619

An ExternalRangeChange does not have the currently mutable fields of PendingRangeChange (and the set of mutable fields is going to expand in the near future). Todos related to limitation of viewing a change as a "change replicas" or "lease transfer" etc., are moved to ExternalRangeChange, so compartmentalized to methods that are called by integration code outside the mma package. There is still some potential future cleanup involving unexporting PendingRangeChange, which is not in scope for this PR. The PendingRangeChange becomes a temporary container for a slice of []*pendingReplicaChanges. Printing this struct no longer involves viewing it as a "change replicas" or "lease transfer". Furthermore, the ReplicaChange.replicaChangeType field is removed and the change type is now a function of prev and next. This sets us up for making prev mutable in a future PR. Informs cockroachdb#157049 Epic: CRDB-55052 Release note: None

sumeerbhola

TFTR!

@sumeerbhola reviewed 3 of 11 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg and @wenyihu6)

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 254 at r1 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

For next PR, a comment on how this might change and that is why we have to keep callling replicaChangeType could be useful.

ack

pkg/kv/kvserver/allocator/mmaprototype/cluster_state.go line 605 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

What does reaching here mean? I assume this is in future PRs, but perhaps we can de-duplicate this logic which is already in ExternalRangeChange.IsTransferLease if it is easy to clean up.

I've changed the code in ExternalRangeChange.IsPureTransferLease to be strict.
And done the same here. And left a todo to see if we can unify.

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 38 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

In the next PR, I’d appreciate a comment explaining that ExternalReplicaChange’s fields are immutable, which is why ChangeType can be static here. It would also be helpful to note how these fields can diverge from PendingRangeChange over time, and that mma cannot use ExternalReplicaChange internally in a meaningful way.

Ack.

pkg/kv/kvserver/allocator/mmaprototype/range_change.go line 247 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

nit: can we align the names isOnlyLeaseChange and IsPureTransferLease? I think one is on ExternalReplicaChange and another is on PendingRangeChange, but they represent the same concept is that right

Done.
One is on ExternalRangeChange and the other is on ExternalReplicaChange.

sumeerbhola requested review from tbg and wenyihu6 November 20, 2025 18:53

sumeerbhola requested review from a team as code owners November 20, 2025 18:53

sumeerbhola mentioned this pull request Nov 20, 2025

mma: eliminate pendingChangeNoRollback #158024

Open

sumeerbhola commented Nov 20, 2025

View reviewed changes

sumeerbhola force-pushed the pending_changes7 branch 2 times, most recently from 2c695de to 0988115 Compare November 20, 2025 21:43

sumeerbhola commented Nov 21, 2025

View reviewed changes

tbg reviewed Nov 21, 2025

View reviewed changes

sumeerbhola force-pushed the pending_changes7 branch from 0988115 to c40bf2f Compare November 21, 2025 14:28

sumeerbhola commented Nov 21, 2025

View reviewed changes

sumeerbhola force-pushed the pending_changes7 branch from c40bf2f to 697d317 Compare November 21, 2025 14:50

sumeerbhola commented Nov 21, 2025

View reviewed changes

sumeerbhola requested a review from tbg November 21, 2025 14:51

mma: move some methods of PendingRangeChange

6477907

This is in preparation for them becoming methods on ExternalRangeChange.

tbg reviewed Nov 21, 2025

View reviewed changes

sumeerbhola force-pushed the pending_changes7 branch from 697d317 to 2c19ef2 Compare November 21, 2025 15:48

wenyihu6 approved these changes Nov 21, 2025

View reviewed changes

sumeerbhola force-pushed the pending_changes7 branch from 2c19ef2 to d65c1d6 Compare November 21, 2025 16:51

sumeerbhola commented Nov 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mma: make external caller mostly use an ExternalRangeChange #158153

mma: make external caller mostly use an ExternalRangeChange #158153

sumeerbhola commented Nov 20, 2025

Uh oh!

cockroach-teamcity commented Nov 20, 2025

Uh oh!

sumeerbhola left a comment

Uh oh!

sumeerbhola left a comment

Uh oh!

tbg left a comment

Uh oh!

tbg commented Nov 21, 2025

Uh oh!

sumeerbhola left a comment

Uh oh!

sumeerbhola left a comment

Uh oh!

tbg left a comment

Uh oh!

wenyihu6 left a comment

Uh oh!

wenyihu6 commented Nov 21, 2025

Uh oh!

sumeerbhola commented Nov 21, 2025

Uh oh!

sumeerbhola left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if kvserverbase.LoadBasedRebalancingModeIsMMA(&as.st.SV) {
	change := convertLeaseTransferToMMA(desc, usage, transferFrom, transferTo)
	mmaChange, isMMARegistered = as.mmaAllocator.RegisterExternalChange(change)
	}

mma: make external caller mostly use an ExternalRangeChange #158153

Are you sure you want to change the base?

mma: make external caller mostly use an ExternalRangeChange #158153

Conversation

sumeerbhola commented Nov 20, 2025

Uh oh!

cockroach-teamcity commented Nov 20, 2025

Uh oh!

sumeerbhola left a comment

Choose a reason for hiding this comment

Uh oh!

sumeerbhola left a comment

Choose a reason for hiding this comment

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

tbg commented Nov 21, 2025

Uh oh!

sumeerbhola left a comment

Choose a reason for hiding this comment

Uh oh!

sumeerbhola left a comment

Choose a reason for hiding this comment

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Nov 21, 2025

Uh oh!

sumeerbhola commented Nov 21, 2025

Uh oh!

sumeerbhola left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants