Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker]fix npe when invoke replaceBookie. #16239

Merged
merged 3 commits into from
Jun 28, 2022

Conversation

Nicklee007
Copy link
Contributor

@Nicklee007 Nicklee007 commented Jun 27, 2022

Motivation

Fix the npe when the ISOLATION_BOOKIE_GROUPS is "" or missed which will produce in follow two case and can be reproduce by the unit test in this PR also.

The npe point

if (isolationGroups != null && isolationGroups.getLeft().contains(PULSAR_SYSTEM_TOPIC_ISOLATION_GROUP)) {

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.getBlacklistedBookiesWithIsolationGroups(IsolatedBookieEnsemblePlacementPolicy.java:220)
	at org.apache.pulsar.bookie.rackawareness.IsolatedBookieEnsemblePlacementPolicy.replaceBookie(IsolatedBookieEnsemblePlacementPolicy.java:170)
	at org.apache.pulsar.bookie.rackawareness.IsolatedBookieEnsemblePlacementPolicyTest.testNoIsolationGroup(IsolatedBookieEnsemblePlacementPolicyTest.java:319)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
	at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
	at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
	at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
	at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
	at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)

Case 1:
When we set strictBookieAffinityEnabled = true and if some namespaces not set bookieAffinityGroup , and then those namespaces will be set ISOLATION_BOOKIE_GROUPS as "" by default, which will cause npe when invoke replaceBookie.

if (serviceConfig.isStrictBookieAffinityEnabled()) {

} else {
Map<String, Object> properties = Maps.newHashMap();
properties.put(IsolatedBookieEnsemblePlacementPolicy.ISOLATION_BOOKIE_GROUPS, "");
properties.put(IsolatedBookieEnsemblePlacementPolicy.SECONDARY_ISOLATION_BOOKIE_GROUPS, "");
managedLedgerConfig.setBookKeeperEnsemblePlacementPolicyProperties(properties);
}

Case 2:
When the bookieAffinityGroup is not null and the ISOLATION_BOOKIE_GROUPS is null or "" also cause npe

Modifications

Describe the modifications you've done.
set a default value when primaryIsolationGroupString is empty.

  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 27, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jun 27, 2022
@codelipenghui codelipenghui added release/2.10.2 release/2.9.4 type/bug The PR fixed a bug or issue reported a bug area/broker labels Jun 27, 2022
@Nicklee007 Nicklee007 requested a review from codelipenghui June 27, 2022 11:33
@Nicklee007
Copy link
Contributor Author

/pulsarbot run-failure-checks

@gaozhangmin gaozhangmin merged commit 0eed842 into apache:master Jun 28, 2022
codelipenghui pushed a commit that referenced this pull request Jun 28, 2022
* fix npe when invoke replaceBookie.

* fix npe when invoke replaceBookie.

* fix npe when invoke replaceBookie.

Co-authored-by: nicklixinyang <nicklixinyang@didiglobal.com>
(cherry picked from commit 0eed842)
mattisonchao pushed a commit that referenced this pull request Jul 2, 2022
* fix npe when invoke replaceBookie.

* fix npe when invoke replaceBookie.

* fix npe when invoke replaceBookie.

Co-authored-by: nicklixinyang <nicklixinyang@didiglobal.com>
(cherry picked from commit 0eed842)
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jul 2, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 4, 2022
* fix npe when invoke replaceBookie.

* fix npe when invoke replaceBookie.

* fix npe when invoke replaceBookie.

Co-authored-by: nicklixinyang <nicklixinyang@didiglobal.com>
(cherry picked from commit 0eed842)
(cherry picked from commit 3048c87)
@Nicklee007 Nicklee007 deleted the fix-replace-bookie-npe branch August 5, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.10.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants