Skip to content

[fix][broker] Prevent incorrect isolation fallback during cluster updates - #26798

Open
Denovo1998 wants to merge 1 commit into
apache:masterfrom
Denovo1998:isolated-bookie-cluster-view
Open

Denovo1998 wants to merge 1 commit into
apache:masterfrom
Denovo1998:isolated-bookie-cluster-view

Conversation

@Denovo1998

Copy link
Copy Markdown
Contributor

Follow-up to the concurrency concern raised in #26701.

Motivation

IsolatedBookieEnsemblePlacementPolicy#getExcludedBookiesWithIsolationGroups reads knownBookies without holding BookKeeper's topology lock, while onClusterChanged modifies this HashMap under the write lock.

A placement request can observe an intermediate topology after a departing primary bookie has been removed but before its replacement has been added. This can incorrectly enable the ungrouped fallback even though the completed cluster update leaves enough writable primary bookies, affecting both newEnsemble and replaceBookie.

Modifications

  • Hold rwLock.readLock() throughout the isolation exclusion calculation so availability counts and exclusions use one consistent topology view.
  • Release the lock in finally, including early-return and exception paths.
  • Extend IsolatedBookieEnsemblePlacementPolicyTest with deterministic regression coverage for both ensemble creation and bookie replacement. The tests use a real in-memory metadata store and pause address resolution during the actual onClusterChanged path, without mocks or direct manipulation of internal state.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Both new regression cases fail on the unpatched implementation because an eligible ungrouped bookie is incorrectly left outside the exclusion set.
  • All 17 test invocations in IsolatedBookieEnsemblePlacementPolicyTest pass with retries disabled.
  • Local validation passed:
    • ./gradlew :pulsar-broker-common:test --tests "IsolatedBookieEnsemblePlacementPolicyTest" -PtestRetryCount=0 -PtestFailFast=false -PtestMaxParallelForks=1 quickCheck
    • git diff --check

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Isolation exclusion calculations now acquire the read lock corresponding to the write lock used by cluster membership updates.

@void-ptr974

void-ptr974 commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Thanks for covering the read of a partially updated topology. There appears to be a remaining gap after the isolation exclusions are calculated: getExcludedBookiesWithIsolationGroups releases the read lock before newEnsemble / replaceBookie call super, where BookKeeper acquires its placement read lock. A cluster change can occur between those two steps.

For an ensemble of two, if only one primary is writable during exclusion calculation, an ungrouped bookie is permitted as fallback. A second primary can join before selection, leaving the ungrouped bookie eligible even though the updated primary group is sufficient. The reverse transition can cause an avoidable BKNotEnoughBookiesException. The new tests cover an update already in progress, but not this interleaving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants