Skip to content

Conversation

@wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Sep 8, 2025

Backport 1/1 commits from #153008.

/cc @cockroachdb/release


Epic: none
Release note: none


kvserver: invoke callback when dropping replicas in bq.SetMaxSize

Previously, when SetMaxSize shrank the queue, replicas were dropped from the
priority queue without invoking their callbacks. This commit ensures callbacks
are properly invoked when SetMaxSize drops replicas. Replicas removed via
removeFromReplicaSetLocked (such as when a replica is destroyed) still don’t
always have their callbacks invoked. While the natural place to invoke the
callback would be at removeFromReplicaSetLocked, invoking callbacks while
holding a lock risks blocking for too long. (We are doing this already for
addInternal though.) This PR focuses specifically on handling the SetMaxSize
case since this PR is intended to be backported. We can follow up with a more
complex but more principled approach on master if needed in the future.

Release justification: low risk fix guarded behind cluster setting

Previously, when SetMaxSize shrank the queue, replicas were dropped from the
priority queue without invoking their callbacks. This commit ensures callbacks
are properly invoked when SetMaxSize drops replicas. Replicas removed via
removeFromReplicaSetLocked (such as when a replica is destroyed) still don’t
always have their callbacks invoked. While the natural place to invoke the
callback would be at removeFromReplicaSetLocked, invoking callbacks while
holding a lock risks blocking for too long. (We are doing this already for
addInternal though.) This PR focuses specifically on handling the SetMaxSize
case since this PR is intended to be backported. We can follow up with a more
complex but more principled approach on master if needed in the future.
@wenyihu6 wenyihu6 requested a review from a team as a code owner September 8, 2025 18:03
@blathers-crl
Copy link

blathers-crl bot commented Sep 8, 2025

Thanks for opening a backport.

Before merging, please confirm that it falls into one of the following categories (select one):

  • Non-production code changes. Includes test-only changes, build system changes, etc.
  • Fixes for serious issues. Defined in the policy as correctness, stability, or security issues, data corruption/loss, significant performance regressions, breaking working and widely used functionality, or an inability to detect and debug production issues.
  • Other approved changes. These changes must be gated behind a disabled-by-default feature flag unless there is a strong justification not to.

Add a brief release justification to the PR description explaining your selection.

Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy.

All backports must be reviewed by the TL and EM for the owning area.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Sep 8, 2025
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl
Copy link

blathers-crl bot commented Sep 8, 2025

❌ PR #153180 does not comply with backport policy

Confidence: medium
Explanation: This PR is forwarded as a backport of a specific change from a master branch PR. The release justification is mentioned, but it is imprecisely identified as 'low risk fix guarded behind cluster setting'. However, no explicit mention or detail on the specific cluster setting or its default state is provided. Since it is only described as 'low risk', this doesn't categorically meet the critical bug fix norm according to the defined policy criteria for immediate backports. Additionally, without clear evidence of a cluster setting being used as a feature flag with a default non-active state, it's not possible to definitively ascertain compliance with the feature flag requirement. Changes are made to the production files 'pkg/kv/kvserver/queue.go' and 'pkg/kv/kvserver/queue_test.go'. The queue_test.go file, being a test, is exempt from feature flag requirements.
Recommendation: Reconsider the backport; Enhance justification or confirm the existence and default state of the cluster setting as a feature flag

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

@wenyihu6 wenyihu6 requested a review from arulajmani September 8, 2025 18:12
@wenyihu6 wenyihu6 merged commit e9b11cf into cockroachdb:release-25.2.6-rc Sep 8, 2025
15 of 16 checks passed
@crl-codesys-jira crl-codesys-jira added the T-admission-control Admission Control label Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Label PR's that are backports to older release branches T-admission-control Admission Control v25.2.6

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants