You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Denovo1998 (via GitHub)" <gi...@apache.org> on 2026/10/01 06:26:30 UTC
[PR] [fix][broker] Prevent incorrect isolation fallback during cluster updates [pulsar]
Denovo1998 opened a new pull request, #26798:
URL: https://github.com/apache/pulsar/pull/26798
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
- [x] 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
- [x] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Prevent incorrect isolation fallback during cluster updates [pulsar]
Posted by "codelipenghui (via GitHub) via commits" <co...@pulsar.apache.org>.
codelipenghui merged PR #26798:
URL: https://github.com/apache/pulsar/pull/26798
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Prevent incorrect isolation fallback during cluster updates [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on PR #26798:
URL: https://github.com/apache/pulsar/pull/26798#issuecomment-5931061144
Thanks for covering the read of a partially updated topology. There is still a gap between calculating the isolation exclusions and selecting bookies: the new read lock is released in `getExcludedBookiesWithIsolationGroups`, then `newEnsemble` / `replaceBookie` call `super`, where BookKeeper acquires its placement read lock.
For example, with ensemble size 2, one writable primary and one ungrouped bookie, the exclusion calculation permits the ungrouped bookie as fallback. If a second primary joins between those two lock acquisitions, placement can use the new topology but still select the ungrouped bookie from the old exclusion decision. The reverse transition can cause an avoidable `BKNotEnoughBookiesException`.
Could the outer `newEnsemble` and `replaceBookie` methods hold the read lock across both exclusion calculation and the `super` call? The lock is reentrant, and concurrent placement readers can still proceed together. A test that lets the cluster update run after exclusion calculation, before the `super` call, would cover this gap; the new tests currently pause inside `onClusterChanged`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Prevent incorrect isolation fallback during cluster updates [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26798:
URL: https://github.com/apache/pulsar/pull/26798#discussion_r4198870288
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -223,6 +223,8 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
if (isolationGroups != null && isolationGroups.getLeft().contains(PULSAR_SYSTEM_TOPIC_ISOLATION_GROUP)) {
return excludedBookies;
}
+ // Keep isolation counts and exclusions on one cluster view while onClusterChanged updates knownBookies.
+ rwLock.readLock().lock();
Review Comment:
**[NIT] a narrower window remains between computing the exclusions and the superclass placement call**
Optional and pre-existing, so fine to leave for a follow-up. The read lock is released when this method returns, but `newEnsemble` and `replaceBookie` call `getExcludedBookies(...)` first and only afterwards enter `super.newEnsemble`/`super.replaceBookie`, which take the read lock again. A `onClusterChanged` that lands in that gap still means the exclusions were computed on the old view; for example a newly joined ungrouped bookie would not be in the exclusion set, even though the primary group is sufficient.
Because the lock is reentrant, `newEnsemble` and `replaceBookie` could take `rwLock.readLock()` around the `getExcludedBookies(...)` call plus the `super` call, and a queued writer would not block the nested read acquisition. This change already closes the case the test covers, so it is not needed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Prevent incorrect isolation fallback during cluster updates [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26798:
URL: https://github.com/apache/pulsar/pull/26798#discussion_r4254847494
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -223,6 +223,8 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
if (isolationGroups != null && isolationGroups.getLeft().contains(PULSAR_SYSTEM_TOPIC_ISOLATION_GROUP)) {
return excludedBookies;
}
+ // Keep isolation counts and exclusions on one cluster view while onClusterChanged updates knownBookies.
+ rwLock.readLock().lock();
Review Comment:
Thanks, this closes the gap. The read lock now covers both the exclusion calculation and the superclass call in `newEnsemble` and `replaceBookie`, and the new test pins it for joins and leaves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Prevent incorrect isolation fallback during cluster updates [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26798:
URL: https://github.com/apache/pulsar/pull/26798#discussion_r4208190212
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -223,6 +223,8 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
if (isolationGroups != null && isolationGroups.getLeft().contains(PULSAR_SYSTEM_TOPIC_ISOLATION_GROUP)) {
return excludedBookies;
}
+ // Keep isolation counts and exclusions on one cluster view while onClusterChanged updates knownBookies.
+ rwLock.readLock().lock();
Review Comment:
Okay, it's already fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org