From eb5e7664c13dade53836ece090b02a44b82089a5 Mon Sep 17 00:00:00 2001 From: Yike Xiao Date: Tue, 19 May 2026 16:56:49 +0800 Subject: [PATCH] [fix][bk] Fix NPE in IsolatedBookieEnsemblePlacementPolicy when policy class does not match MutablePair was initialized without default values, leaving left/right as null when the ensemble placement policy class name did not match. This caused a NullPointerException when getLeft().contains() was called. Co-Authored-By: Claude Sonnet 4.6 --- ...IsolatedBookieEnsemblePlacementPolicy.java | 21 +++---- ...atedBookieEnsemblePlacementPolicyTest.java | 57 +++++++++++++++++++ 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java index fc850516462da..f920b9647c1dc 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java @@ -40,6 +40,7 @@ import org.apache.bookkeeper.net.DNSToSwitchMapping; import org.apache.bookkeeper.proto.BookieAddressResolver; import org.apache.bookkeeper.stats.StatsLogger; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.tuple.ImmutablePair; import org.apache.commons.lang3.tuple.MutablePair; import org.apache.commons.lang3.tuple.Pair; @@ -168,7 +169,7 @@ private static Optional getEnsemblePlacementPolic private static Pair, Set> getIsolationGroup( EnsemblePlacementPolicyConfig ensemblePlacementPolicyConfig) { - MutablePair, Set> pair = new MutablePair<>(); + MutablePair, Set> pair = new MutablePair<>(Collections.emptySet(), Collections.emptySet()); String className = IsolatedBookieEnsemblePlacementPolicy.class.getName(); if (ensemblePlacementPolicyConfig.getPolicyClass().getName().equals(className)) { Map properties = ensemblePlacementPolicyConfig.getProperties(); @@ -178,13 +179,9 @@ private static Pair, Set> getIsolationGroup( .castToString(properties.getOrDefault(SECONDARY_ISOLATION_BOOKIE_GROUPS, "")); if (!primaryIsolationGroupString.isEmpty()) { pair.setLeft(Sets.newHashSet(primaryIsolationGroupString.split(","))); - } else { - pair.setLeft(Collections.emptySet()); } if (!secondaryIsolationGroupString.isEmpty()) { pair.setRight(Sets.newHashSet(secondaryIsolationGroupString.split(","))); - } else { - pair.setRight(Collections.emptySet()); } } return pair; @@ -193,8 +190,14 @@ private static Pair, Set> getIsolationGroup( @VisibleForTesting Set getExcludedBookiesWithIsolationGroups(int ensembleSize, Pair, Set> isolationGroups) { + Set primaryIsolationGroup = Collections.emptySet(); + Set secondaryIsolationGroup = Collections.emptySet(); + if (isolationGroups != null) { + primaryIsolationGroup = ObjectUtils.getIfNull(isolationGroups.getLeft(), Collections.emptySet()); + secondaryIsolationGroup = ObjectUtils.getIfNull(isolationGroups.getRight(), Collections.emptySet()); + } Set excludedBookies = new HashSet<>(); - if (isolationGroups != null && isolationGroups.getLeft().contains(PULSAR_SYSTEM_TOPIC_ISOLATION_GROUP)) { + if (primaryIsolationGroup.contains(PULSAR_SYSTEM_TOPIC_ISOLATION_GROUP)) { return excludedBookies; } try { @@ -215,13 +218,7 @@ Set getExcludedBookiesWithIsolationGroups(int ensembleSize, return excludedBookies; } int totalAvailableBookiesInPrimaryGroup = 0; - Set primaryIsolationGroup = Collections.emptySet(); - Set secondaryIsolationGroup = Collections.emptySet(); Set primaryGroupBookies = new HashSet<>(); - if (isolationGroups != null) { - primaryIsolationGroup = isolationGroups.getLeft(); - secondaryIsolationGroup = isolationGroups.getRight(); - } for (String group : allGroups) { Set bookiesInGroup = allGroupsBookieMapping.get(group).keySet(); if (!primaryIsolationGroup.contains(group)) { diff --git a/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicyTest.java b/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicyTest.java index 7940ccc636bf6..7dd8edc99aef3 100644 --- a/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicyTest.java +++ b/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicyTest.java @@ -43,6 +43,7 @@ import java.util.Set; import java.util.concurrent.CompletableFuture; import org.apache.bookkeeper.client.BKException.BKNotEnoughBookiesException; +import org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy; import org.apache.bookkeeper.conf.ClientConfiguration; import org.apache.bookkeeper.feature.SettableFeatureProvider; import org.apache.bookkeeper.net.BookieId; @@ -839,6 +840,62 @@ public void testGetExcludedBookiesWithIsolationGroups() throws Exception { assertTrue(blacklist.isEmpty()); } + /** + * Regression test for the NPE reported in the stack trace below. When custom metadata carries an + * {@link EnsemblePlacementPolicyConfig} whose policy class does NOT match + * {@link IsolatedBookieEnsemblePlacementPolicy}, the old {@code getIsolationGroup()} returned a + * {@code MutablePair} with {@code null} left/right, which caused a {@link NullPointerException} in + * {@code getExcludedBookiesWithIsolationGroups} when {@code getLeft().contains(...)} was called. + * + * 
+     * 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 IsolatedBookieEnsemblePlacementPolicy.getExcludedBookiesWithIsolationGroups(...)
+     *     at IsolatedBookieEnsemblePlacementPolicy.getExcludedBookies(...)
+     *     at IsolatedBookieEnsemblePlacementPolicy.replaceBookie(...)
+     *
+ */ + @Test + public void testReplaceBookieWithNonMatchingPolicyClassShouldNotThrowNPE() throws Exception { + Map> bookieMapping = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put(BOOKIE1, BookieInfo.builder().rack("rack0").build()); + group1.put(BOOKIE2, BookieInfo.builder().rack("rack1").build()); + group1.put(BOOKIE3, BookieInfo.builder().rack("rack0").build()); + group1.put(BOOKIE4, BookieInfo.builder().rack("rack1").build()); + bookieMapping.put("group1", group1); + + store.put(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH, jsonMapper.writeValueAsBytes(bookieMapping), + Optional.empty()).join(); + + IsolatedBookieEnsemblePlacementPolicy isolationPolicy = new IsolatedBookieEnsemblePlacementPolicy(); + ClientConfiguration bkClientConf = new ClientConfiguration(); + bkClientConf.setProperty(BookieRackAffinityMapping.METADATA_STORE_INSTANCE, store); + bkClientConf.setProperty(IsolatedBookieEnsemblePlacementPolicy.ISOLATION_BOOKIE_GROUPS, "group1"); + isolationPolicy.initialize(bkClientConf, Optional.empty(), timer, SettableFeatureProvider.DISABLE_ALL, + NullStatsLogger.INSTANCE, BookieSocketAddress.LEGACY_BOOKIEID_RESOLVER); + isolationPolicy.onClusterChanged(writableBookies, readOnlyBookies); + + // Use a policy class that does NOT match IsolatedBookieEnsemblePlacementPolicy. + // In the old code this caused getIsolationGroup() to return a MutablePair with null left/right, + // triggering NPE at the getLeft().contains() call in getExcludedBookiesWithIsolationGroups. + EnsemblePlacementPolicyConfig policyConfig = new EnsemblePlacementPolicyConfig( + RackawareEnsemblePlacementPolicy.class, Collections.emptyMap()); + Map customMetadata = new HashMap<>(); + customMetadata.put(EnsemblePlacementPolicyConfig.ENSEMBLE_PLACEMENT_POLICY_CONFIG, policyConfig.encode()); + + BookieId bookie1Id = new BookieSocketAddress(BOOKIE1).toBookieId(); + BookieId bookie2Id = new BookieSocketAddress(BOOKIE2).toBookieId(); + + try { + // Must not throw NullPointerException; BKNotEnoughBookiesException is acceptable. + isolationPolicy.replaceBookie(1, 1, 1, customMetadata, + Arrays.asList(bookie1Id, bookie2Id), bookie2Id, null); + } catch (BKNotEnoughBookiesException e) { + // acceptable: no bookies match the (empty) isolation group, but no NPE occurred + } + } + // The policy gets the bookie info asynchronously before each query or update, when putting the bookie info into // the metadata store, the cache needs some time to receive the notification and update accordingly. private void updateBookieInfo(IsolatedBookieEnsemblePlacementPolicy isolationPolicy, byte[] bookieInfo) {