fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver #2788

gaozhangmin · 2021-09-08T04:26:39Z

Error log:

16:21:20.140 [main] ERROR org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl - Failed to initialize DNS Resolver org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping, used default subnet resolver : java.lang.RuntimeException: java.lang.NullPointerException java.lang.NullPointerException

BookieAddressResolver should be set before ((Configurable) dnsResolver).setConf(conf);

It will throw npe. when pulsar ZkBookieRackAffinityMapping invoke getBookieAddressResolver

gaozhangmin · 2021-09-08T04:27:15Z

@hangc0276 PTAL

eolivelli

+1

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java

nicoloboschi

LGTM

gaozhangmin · 2021-09-08T06:32:44Z

pulsar also has a related issue apache/pulsar#11947
@eolivelli @lhotari @nicoloboschi PTAL

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java

lhotari

LGTM

lhotari · 2021-09-08T07:13:01Z

@gaozhangmin Can you add the stacktrace containing the NPE to the description? That could help find this PR if someone searches by stacktrace elements.

gaozhangmin · 2021-09-08T07:42:56Z

@gaozhangmin Can you add the stacktrace containing the NPE to the description? That could help find this PR if someone searches by stacktrace elements.

there no stacktrace in log , only java.lang.nullpointexception

lhotari · 2021-09-08T08:08:34Z

there no stacktrace in log , only java.lang.nullpointexception

The log line with that could also help locate this issue when someone with the same problem searches for it. Please share.

gaozhangmin · 2021-09-08T08:23:23Z

there no stacktrace in log , only java.lang.nullpointexception

The log line with that could also help locate this issue when someone with the same problem searches for it. Please share.

added

lhotari · 2021-09-08T08:37:51Z

Thanks for providing the log message @gaozhangmin . It looks like the logging is broken since the provided sample log message didn't include the stacktrace. I'd propose to fix it in the same PR since it's closely related to the change that is being made.

This looks invalid:
LOG.error("Failed to initialize DNS Resolver {}, used default subnet resolver : {} {}", dnsResolverName, re, re.getMessage());

I wonder why it's not simply
LOG.error("Failed to initialize DNS Resolver {}, using default subnet resolver.", dnsResolverName, re); ?

gaozhangmin · 2021-09-08T09:09:59Z

Thanks for providing the log message @gaozhangmin . It looks like the logging is broken since the provided sample log message didn't include the stacktrace. I'd propose to fix it in the same PR since it's closely related to the change that is being made.

This looks invalid:
LOG.error("Failed to initialize DNS Resolver {}, used default subnet resolver : {} {}", dnsResolverName, re, re.getMessage());

I wonder why it's not simply
LOG.error("Failed to initialize DNS Resolver {}, using default subnet resolver.", dnsResolverName, re); ?

changed

lhotari

LGTM

dlg99

Can I ask you to:

configure dnsResolver where it is created/before passing it to downstream components instead of setting config to the instance passed here (this can have side-effects, I don't remember if we have to worry about thread safety in that setter etc.) See my comment on the code.
Overall I'd assume that component should get a usable parameter value instead of a half-baked one, so patching the configuration there smells like a hack instead of a fix.
In case that's not possible for some reason I'd love to understand why (to be fair, i didn't look at the code that deep to remember all nuances).
add unit-tests
check other policies (region-aware/zone-aware) to make sure these problems don't exist there.

i.e. in ZoneawareEnsemblePlacementPolicyImpl I see

        DNSToSwitchMapping actualDNSResolver;
        if (optionalDnsResolver.isPresent()) {
            actualDNSResolver = optionalDnsResolver.get();
        } else {

I assume that we'll be fine if we pass properly configured resolver there.
But if there is an issue that prevents us from doing so, both policies will have similar problems.

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java

eolivelli · 2021-09-09T05:55:27Z

Canbyou add a test cases that shows the problem?
This way it will be clear what we are fixing and that you fixed it.
I would prefer to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK

gaozhangmin · 2021-09-09T06:19:27Z

I would prefer to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK

This need pulsar org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping class, I do not know how to test this.

eolivelli · 2021-09-09T06:36:02Z

Sorry I meant:

I would NOT prefer to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK

So this change in this current form works for me.
That said, adding a simple test case that reproduces the NPE will be better, otherwise we could break ZkBookieRackAffinityMapping again

If you have the error on ZkBookieRackAffinityMapping you could try to use the bookieAddressResolver in the way that ZkBookieRackAffinityMapping does, but in a dummy implementation

gaozhangmin · 2021-09-09T06:53:06Z

Sorry I meant:

I would NOT prefer to refactor interfaces or pluggable components in order to not introduce API changes, in that case it would be harder to upgrade BK

So this change in this current form works for me.
That said, adding a simple test case that reproduces the NPE will be better, otherwise we could break ZkBookieRackAffinityMapping again

If you have the error on ZkBookieRackAffinityMapping you could try to use the bookieAddressResolver in the way that ZkBookieRackAffinityMapping does, but in a dummy implementation

Unit test added, PTAL

eolivelli

I believe we are on our way.

Is it possible to add some test cases, or enhance some existing test cases, in order to cover the changes you made ?

gaozhangmin · 2021-09-09T10:00:06Z

I believe we are on our way.

Is it possible to add some test cases, or enhance some existing test cases, in order to cover the changes you made ?

I think the unit test TestRackawareEnsemblePlacementPolicy already covered this change.

like repp.initialize(conf, Optional.<DNSToSwitchMapping>empty(), timer, DISABLE_ALL, NullStatsLogger.INSTANCE, BookieSocketAddress.LEGACY_BOOKIEID_RESOLVER);

the dnsResolver is null, will reach the condition !optionalDnsResolver.isPresent()

dlg99

Looks awesome!
Thank you for going extra mile with the tests.

gaozhangmin · 2021-09-20T13:49:30Z

any problem with this pr?

eolivelli

Lgtm

eolivelli · 2021-09-20T13:52:39Z

We will merge as soon as CI passes.

gaozhangmin · 2021-09-20T14:56:34Z

The failed tests seems coming from timeouts. it's not related with this change.

gaozhangmin · 2021-09-24T03:32:50Z

@eolivelli Please approve running workflows

gaozhangmin · 2021-09-27T06:29:03Z

@dlg99 Please help approve runn workflows

gaozhangmin · 2021-09-28T03:43:36Z

@eolivelli the failed tests come from timeouts, please rerun failed tests

gaozhangmin · 2021-10-08T02:41:47Z

@eolivelli CI passes. please merge this pr

gaozhangmin · 2021-10-13T12:05:32Z

@eolivelli

…ver (#2788) Error log: `16:21:20.140 [main] ERROR org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl - Failed to initialize DNS Resolver org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping, used default subnet resolver : java.lang.RuntimeException: java.lang.NullPointerException java.lang.NullPointerException` `BookieAddressResolver` should be set before `((Configurable) dnsResolver).setConf(conf);` It will throw npe. when pulsar `ZkBookieRackAffinityMapping` invoke getBookieAddressResolver (cherry picked from commit 031d168)

### Motivation In order to complete migration to Gradle we must build all the subprojects. ### Changes - Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle` - Added these modules to the build - `bookkeeper-http:servlet-http-server` - `metadata-drivers:etcd` - `tests:backward-compat:*` - `tests:shaded:*` - `stream:bk-grpc-name-resolver` - DL shading process is now performed (before it didn't build any jar) - Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped ### Test - Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally - Tests are passing locally Master Issue: #2849 Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None> This closes #2850 from nicoloboschi/fix/2849/gradle and squashes the following commits: 00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex bd739fd [Nicolò Boschi] fix sh tests 43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration d1f95e4 [Nicolò Boschi] fix shaded deps bcab40d [Nicolò Boschi] fix build 5fd0341 [Nicolò Boschi] fix build 0082e0e [Nicolò Boschi] fix build 2c32ac1 [Nicolò Boschi] fixes 3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests ba89132 [Nicolò Boschi] shaded tests 6d39e33 [Nicolò Boschi] sh tests e0032bc [Nicolò Boschi] actually run arquillian groovy tests 08dcc39 [Nicolò Boschi] backwards 2361f79 [Nicolò Boschi] hierarchical-ledger-manager 8388e11 [Nicolò Boschi] current-server-old-clients 6a24344 [Nicolò Boschi] bc-non-fips 2faca01 [Nicolò Boschi] bk-grpc-name-resolver 991bc11 [Nicolò Boschi] servlet-http-server 675ef7b [Nicolò Boschi] etcd b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (#2845) bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (#2844) 746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (#2842) 4117200 [ZhangJian He] the compare should be >= instead of > (#2782) 14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (#2843) e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (#2819) 53954ca [shustsud] Add ensemble check to over-replicated ledger GC (#2813) 919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (#2841) 031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (#2788) 3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (#2812) f6903b8 [Jack Vanlightly] BP-44 USE metrics a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (#2833) a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11

…ver (apache#2788) Error log: `16:21:20.140 [main] ERROR org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl - Failed to initialize DNS Resolver org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping, used default subnet resolver : java.lang.RuntimeException: java.lang.NullPointerException java.lang.NullPointerException` `BookieAddressResolver` should be set before `((Configurable) dnsResolver).setConf(conf);` It will throw npe. when pulsar `ZkBookieRackAffinityMapping` invoke getBookieAddressResolver

### Motivation In order to complete migration to Gradle we must build all the subprojects. ### Changes - Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle` - Added these modules to the build - `bookkeeper-http:servlet-http-server` - `metadata-drivers:etcd` - `tests:backward-compat:*` - `tests:shaded:*` - `stream:bk-grpc-name-resolver` - DL shading process is now performed (before it didn't build any jar) - Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped ### Test - Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally - Tests are passing locally Master Issue: apache#2849 Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None> This closes apache#2850 from nicoloboschi/fix/2849/gradle and squashes the following commits: 00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex bd739fd [Nicolò Boschi] fix sh tests 43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration d1f95e4 [Nicolò Boschi] fix shaded deps bcab40d [Nicolò Boschi] fix build 5fd0341 [Nicolò Boschi] fix build 0082e0e [Nicolò Boschi] fix build 2c32ac1 [Nicolò Boschi] fixes 3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests ba89132 [Nicolò Boschi] shaded tests 6d39e33 [Nicolò Boschi] sh tests e0032bc [Nicolò Boschi] actually run arquillian groovy tests 08dcc39 [Nicolò Boschi] backwards 2361f79 [Nicolò Boschi] hierarchical-ledger-manager 8388e11 [Nicolò Boschi] current-server-old-clients 6a24344 [Nicolò Boschi] bc-non-fips 2faca01 [Nicolò Boschi] bk-grpc-name-resolver 991bc11 [Nicolò Boschi] servlet-http-server 675ef7b [Nicolò Boschi] etcd b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (apache#2845) bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (apache#2844) 746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (apache#2842) 4117200 [ZhangJian He] the compare should be >= instead of > (apache#2782) 14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (apache#2843) e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (apache#2819) 53954ca [shustsud] Add ensemble check to over-replicated ledger GC (apache#2813) 919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (apache#2841) 031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (apache#2788) 3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (apache#2812) f6903b8 [Jack Vanlightly] BP-44 USE metrics a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (apache#2833) a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (apache#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (apache#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (apache#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (apache#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (apache#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (apache#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (apache#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11

eolivelli approved these changes Sep 8, 2021

View reviewed changes

lhotari reviewed Sep 8, 2021

View reviewed changes

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java Outdated Show resolved Hide resolved

nicoloboschi approved these changes Sep 8, 2021

View reviewed changes

lhotari requested changes Sep 8, 2021

View reviewed changes

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java Show resolved Hide resolved

lhotari approved these changes Sep 8, 2021

View reviewed changes

dlg99 requested changes Sep 8, 2021

View reviewed changes

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java Outdated Show resolved Hide resolved

gaozhangmin requested review from eolivelli and dlg99 September 9, 2021 07:11

eolivelli reviewed Sep 9, 2021

View reviewed changes

dlg99 approved these changes Sep 11, 2021

View reviewed changes

casuallc mentioned this pull request Sep 14, 2021

NPE while preparing racksandhost detail through RackawareEnsemblePlacementPolicyImpl apache/pulsar#11972

Closed

gaozhangmin force-pushed the fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver branch from e506095 to 9f2bddb Compare September 16, 2021 03:07

eolivelli approved these changes Sep 20, 2021

View reviewed changes

fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver

497c9db

gaozhangmin force-pushed the fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver branch from 9f2bddb to 497c9db Compare September 24, 2021 02:22

zymap assigned gaozhangmin Oct 20, 2021

zymap added the release/4.14.3 label Oct 20, 2021

zymap merged commit 031d168 into apache:master Oct 21, 2021

michaeljmarshall mentioned this pull request Jul 27, 2022

Fix rack awareness cache expiration data race apache/pulsar#16825

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver #2788

fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver #2788

gaozhangmin commented Sep 8, 2021 •

edited

Loading

gaozhangmin commented Sep 8, 2021

eolivelli left a comment

nicoloboschi left a comment

gaozhangmin commented Sep 8, 2021

lhotari left a comment

lhotari commented Sep 8, 2021 •

edited

Loading

gaozhangmin commented Sep 8, 2021

lhotari commented Sep 8, 2021

gaozhangmin commented Sep 8, 2021

lhotari commented Sep 8, 2021 •

edited

Loading

gaozhangmin commented Sep 8, 2021

lhotari left a comment

dlg99 left a comment

eolivelli commented Sep 9, 2021

gaozhangmin commented Sep 9, 2021

eolivelli commented Sep 9, 2021 •

edited

Loading

gaozhangmin commented Sep 9, 2021

eolivelli left a comment

gaozhangmin commented Sep 9, 2021

dlg99 left a comment

gaozhangmin commented Sep 20, 2021

eolivelli left a comment

eolivelli commented Sep 20, 2021

gaozhangmin commented Sep 20, 2021

gaozhangmin commented Sep 24, 2021

gaozhangmin commented Sep 27, 2021

gaozhangmin commented Sep 28, 2021

gaozhangmin commented Oct 8, 2021

gaozhangmin commented Oct 13, 2021

fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver #2788

fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver #2788

Conversation

gaozhangmin commented Sep 8, 2021 • edited Loading

gaozhangmin commented Sep 8, 2021

eolivelli left a comment

Choose a reason for hiding this comment

nicoloboschi left a comment

Choose a reason for hiding this comment

gaozhangmin commented Sep 8, 2021

lhotari left a comment

Choose a reason for hiding this comment

lhotari commented Sep 8, 2021 • edited Loading

gaozhangmin commented Sep 8, 2021

lhotari commented Sep 8, 2021

gaozhangmin commented Sep 8, 2021

lhotari commented Sep 8, 2021 • edited Loading

gaozhangmin commented Sep 8, 2021

lhotari left a comment

Choose a reason for hiding this comment

dlg99 left a comment

Choose a reason for hiding this comment

eolivelli commented Sep 9, 2021

gaozhangmin commented Sep 9, 2021

eolivelli commented Sep 9, 2021 • edited Loading

gaozhangmin commented Sep 9, 2021

eolivelli left a comment

Choose a reason for hiding this comment

gaozhangmin commented Sep 9, 2021

dlg99 left a comment

Choose a reason for hiding this comment

gaozhangmin commented Sep 20, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Sep 20, 2021

gaozhangmin commented Sep 20, 2021

gaozhangmin commented Sep 24, 2021

gaozhangmin commented Sep 27, 2021

gaozhangmin commented Sep 28, 2021

gaozhangmin commented Oct 8, 2021

gaozhangmin commented Oct 13, 2021

gaozhangmin commented Sep 8, 2021 •

edited

Loading

lhotari commented Sep 8, 2021 •

edited

Loading

lhotari commented Sep 8, 2021 •

edited

Loading

eolivelli commented Sep 9, 2021 •

edited

Loading