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 #153179 is compliant with backport policy

Confidence: high
Feature flag detected: Yes
Backward compatible: true
Explanation: The PR is compliant with the CockroachDB backport policy. Firstly, the PR body includes a 'Release justification: low risk fix guarded behind cluster setting' line, which provides a valid reason for the backport, exempting it from the standard policy requirements of needing to fix a critical bug or being gated behind a feature flag. Secondly, although the primary change affects production code, it is described as a low-risk fix and is explicitly mentioned to be guarded by a cluster setting. This suggest compliance with the feature flag requirement as new cluster settings controlling behavior are treated as a form of feature flag gating. Additionally, the PR does not indicate any back-breaking changes or the removal of a version gate, supporting backward compatibility.

🦉 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 917f8fe into cockroachdb:release-24.3.20-rc Sep 8, 2025
16 checks passed
@wenyihu6 wenyihu6 deleted the backportrelease-24.3.20-rc-153008 branch September 8, 2025 19:55
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-kv-replication v24.3.20

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants