Skip to content

Conversation

@wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Aug 26, 2025

Epic: none
Release note: none


allocator: move isDecommissionAction to allocatorimpl

This commit refactors isDecommissionAction into allocatorimpl for consistency
with other similar helpers like allocatorActions.{Add,Replace,Remove}. This
change has no behavior changes but to make future commits easier.


kvserver: simplify logging in maybeEnqueueProblemRange

This commit simplifies the logging in maybeEnqueueProblemRange to log two
booleans directly.


kvserver: fix comment when dropping due to exceeding size

Previously, the comment on the queue incorrectly stated that it removes the
lowest-priority element when exceeding its maximum size. This was misleading
because heap only guarantees that the root is the highest priority, not that
elements are globally ordered. This commit updates the comment to clarify that
the removed element might not be the lowest priority. Ideally, we should drop
the lowest priority element when exceeding queue size, but heap doesn't make
this very easy.


kvserver: add logging for ranges dropped from base queue

This commit adds logging for ranges dropped from the base queue due to exceeding
max size, improving observability. The log is gated behind V(1) to avoid
verbosity on nodes with many ranges.

@blathers-crl
Copy link

blathers-crl bot commented Aug 26, 2025

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@wenyihu6 wenyihu6 force-pushed the smallchanges branch 5 times, most recently from 1576079 to 5579035 Compare August 26, 2025 13:08
@wenyihu6 wenyihu6 marked this pull request as ready for review August 26, 2025 13:09
@wenyihu6 wenyihu6 requested review from a team as code owners August 26, 2025 13:09
@wenyihu6 wenyihu6 requested review from arulajmani and tbg August 26, 2025 13:09
Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

:lgtm: modulo comments

@arulajmani reviewed 2 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)


-- commits line 33 at r5:
Did you mean to keep this fixup?


pkg/kv/kvserver/queue.go line 776 at r4 (raw file):

	if pqLen := bq.mu.priorityQ.Len(); pqLen > bq.maxSize {
		replicaItemToDrop := bq.mu.priorityQ.sl[pqLen-1]
		log.Dev.VInfof(ctx, 1, "dropping due to exceeding queue max size: priority=%0.3f, replica=%v",

nit: Should this be in KVDistribution?

@wenyihu6
Copy link
Contributor Author

pkg/kv/kvserver/queue.go line 776 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: Should this be in KVDistribution?

Hm, was wondering abt this as well. But other logs in this file use log.Dev, and this is for base queue which is used for other queues outside of kv distribution, such as mvccGC.

This commit refactors isDecommissionAction into allocatorimpl for consistency
with other similar helpers like allocatorActions.{Add,Replace,Remove}. This
change has no behavior changes but to make future commits easier.
This commit simplifies the logging in `maybeEnqueueProblemRange` to log two
booleans directly.
Previously, the comment on the queue incorrectly stated that it removes the
lowest-priority element when exceeding its maximum size. This was misleading
because heap only guarantees that the root is the highest priority, not that
elements are globally ordered. This commit updates the comment to clarify that
the removed element might not be the lowest priority. Ideally, we should drop
the lowest priority element when exceeding queue size, but heap doesn't make
this very easy.
This commit adds logging for ranges dropped from the base queue due to exceeding
max size, improving observability. The log is gated behind V(1) to avoid
verbosity on nodes with many ranges.
@wenyihu6
Copy link
Contributor Author

Rebased on master and picked up the fixup.

@wenyihu6
Copy link
Contributor Author

TFTR!

bors r=arulajmani

@craig
Copy link
Contributor

craig bot commented Aug 26, 2025

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