You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Shawyeok (via GitHub)" <gi...@apache.org> on 2026/05/19 15:53:53 UTC
[PR] [fix][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Shawyeok opened a new pull request, #25825:
URL: https://github.com/apache/pulsar/pull/25825
Fixes NPE in `IsolatedBookieEnsemblePlacementPolicy`
### Motivation
A `NullPointerException` occurs in `getExcludedBookiesWithIsolationGroups` when `getIsolationGroup()` returns a `Pair` whose `left` value is `null`. This happens when the `EnsemblePlacementPolicyConfig` carries a policy class name that does not equal `IsolatedBookieEnsemblePlacementPolicy` — in that case the `if` block that calls `pair.setLeft()` / `pair.setRight()` is skipped entirely, leaving both sides of the `MutablePair` as `null`.
This bug was observed during the upgrade from the 2.8 branch to the 3.0 branch. The same defect may also exist in master.
**Full stack trace from production:**
```
2026-05-18T13:28:40.756Z [BookKeeperClientWorker-OrderedExecutor-35-0] ERROR org.apache.bookkeeper.common.util.SingleThreadExecutor - Error while running task: Cannot invoke "java.util.Set.contains(Object)" because the return value of "org.apache.commons.lang3.tuple.Pair.getLeft()" is null
java.lang.NullPointerException: Cannot invoke "java.util.Set.contains(Object)" because the return value of "org.apache.commons.lang3.tuple.Pair.getLeft()" is null
at org.apache.pulsar.bookie.rackawareness.IsolatedBookieEnsemblePlacementPolicy.getExcludedBookiesWithIsolationGroups(IsolatedBookieEnsemblePlacementPolicy.java:192)
at org.apache.pulsar.bookie.rackawareness.IsolatedBookieEnsemblePlacementPolicy.getExcludedBookies(IsolatedBookieEnsemblePlacementPolicy.java:141)
at org.apache.pulsar.bookie.rackawareness.IsolatedBookieEnsemblePlacementPolicy.replaceBookie(IsolatedBookieEnsemblePlacementPolicy.java:127)
at org.apache.bookkeeper.client.BookieWatcherImpl.replaceBookie(BookieWatcherImpl.java:316)
at org.apache.bookkeeper.client.EnsembleUtils.replaceBookiesInEnsemble(EnsembleUtils.java:69)
at org.apache.bookkeeper.client.ReadOnlyLedgerHandle.handleBookieFailure(ReadOnlyLedgerHandle.java:222)
at org.apache.bookkeeper.client.PendingAddOp.writeComplete(PendingAddOp.java:353)
at org.apache.bookkeeper.proto.BookieClientImpl.completeAdd(BookieClientImpl.java:287)
at org.apache.bookkeeper.proto.BookieClientImpl.access$200(BookieClientImpl.java:79)
at org.apache.bookkeeper.proto.BookieClientImpl$ChannelReadyForAddEntryCallback.lambda$operationComplete$0(BookieClientImpl.java:405)
at org.apache.bookkeeper.common.util.OrderedExecutor$TimedRunnable.run(OrderedExecutor.java:203)
at org.apache.bookkeeper.common.util.SingleThreadExecutor.safeRunTask(SingleThreadExecutor.java:137)
at org.apache.bookkeeper.common.util.SingleThreadExecutor.run(SingleThreadExecutor.java:107)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:840)
```
This error causes topics to time out during loading. Since there is no automatic recovery path, the only way to restore service is by restarting the broker.
### Modifications
Two complementary fixes:
1. **`getIsolationGroup()`**: Initialize `MutablePair` with empty sets as defaults so the returned `Pair` always has non-null values, and remove the now-redundant `else` branches that were explicitly setting the same empty sets.
2. **`getExcludedBookiesWithIsolationGroups()`** (defensive programming): Resolve `primaryIsolationGroup` and `secondaryIsolationGroup` at the top of the method using `ObjectUtils.getIfNull`, before any use. This eliminates both the original NPE at the early-return `contains` check and a second latent NPE at lines 218–219 where the pair values were previously assigned and dereferenced without null guards.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- Added `testReplaceBookieWithNonMatchingPolicyClassShouldNotThrowNPE` to reproduce the exact production failure: calling `replaceBookie` with custom metadata whose `EnsemblePlacementPolicyConfig` policy class does not match `IsolatedBookieEnsemblePlacementPolicy`, which previously caused the NPE.
### Does this pull request potentially affect one of the following parts:
- [ ] 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
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "Shawyeok (via GitHub)" <gi...@apache.org>.
Shawyeok commented on PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#issuecomment-4525536273
/pulsarbot rerun-failure-checks
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "Shawyeok (via GitHub)" <gi...@apache.org>.
Shawyeok commented on code in PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#discussion_r3286250201
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
@void-ptr974
Good point! The tricky part is that handling of empty isolation group in IsolatedBookieEnsemblePlacementPolicy. Now it as treated as negative for all bookies.
Currently, we can only call the `getExcludedBookiesWithIsolationGroups` method before resolving any possible pair of null scenarios.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
@void-ptr974
Good point! The tricky part is that handling of empty isolation group in `IsolatedBookieEnsemblePlacementPolicy`. Now it as treated as negative for all bookies.
Currently, we can only call the `getExcludedBookiesWithIsolationGroups` method before resolving any possible pair of null scenarios.
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#discussion_r3272428690
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
@Shawyeok Please check this. I wonder if there's a test case that would cover the explained scenario?
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun merged PR #25825:
URL: https://github.com/apache/pulsar/pull/25825
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "Shawyeok (via GitHub)" <gi...@apache.org>.
Shawyeok commented on code in PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#discussion_r3286250201
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
@void-ptr974
Good point! The tricky part is that handling of empty isolation group in IsolatedBookieEnsemblePlacementPolicy. Now it as treated as negative for all bookies.
Currently, we can only call the getExcludedBookiesWithIsolationGroups method before resolving any possible pair of null scenarios.
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "Shawyeok (via GitHub)" <gi...@apache.org>.
Shawyeok commented on PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#issuecomment-4551458081
@lhotari @dao-jun
All tests have passed, feel free to take a look, thanks.
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#discussion_r3271485201
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
This is the problematic behavior for the non-matching policyClass case. After this PR, getIsolationGroup() returns empty primary/secondary groups when the metadata policyClass is not
IsolatedBookieEnsemblePlacementPolicy. With primaryIsolationGroup empty, this condition is true for every configured group, so all grouped bookies are added to excludedBookies below. That avoids the NPE but can
still make replaceBookie fail with BKNotEnoughBookiesException. A non-matching policyClass should be ignored or fall back to defaultIsolationGroups, not be treated as an empty isolation group.
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "Shawyeok (via GitHub)" <gi...@apache.org>.
Shawyeok commented on code in PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#discussion_r3286467305
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
I've made a change of fallback to use defaultIsolationGroups when policy class doesn't match. Feel free to take a look. @void-ptr974 @lhotari
--
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][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match [pulsar]
Posted by "Shawyeok (via GitHub)" <gi...@apache.org>.
Shawyeok commented on code in PR #25825:
URL: https://github.com/apache/pulsar/pull/25825#discussion_r3286467305
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java:
##########
@@ -215,13 +218,7 @@ Set<BookieId> getExcludedBookiesWithIsolationGroups(int ensembleSize,
return excludedBookies;
}
int totalAvailableBookiesInPrimaryGroup = 0;
- Set<String> primaryIsolationGroup = Collections.emptySet();
- Set<String> secondaryIsolationGroup = Collections.emptySet();
Set<BookieId> primaryGroupBookies = new HashSet<>();
- if (isolationGroups != null) {
- primaryIsolationGroup = isolationGroups.getLeft();
- secondaryIsolationGroup = isolationGroups.getRight();
- }
for (String group : allGroups) {
Set<String> bookiesInGroup = allGroupsBookieMapping.get(group).keySet();
if (!primaryIsolationGroup.contains(group)) {
Review Comment:
I've made a change of fallback to use defaultIsolationGroups when policy class doesn't match.
--
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