Bugfix: Fix rackaware placement policy init error #12097

addisonj · 2021-09-19T20:46:18Z

Since the release of Pulsar 2.8 and upgrade to BK 4.12, the default
rackAwarePlacementPolicy has been failing with the following exception:

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

This regression occured in commit 4c60262

The core of the issue is that setConf is called before
setBookieAddressResolver has been set (see
https://github.com/apache/bookkeeper/blob/034ef8566ad037937a4d58a28f70631175744f53/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L264-L286)

This results in the NPE.

We are safe to simply not eagerly init the cache in setConf as the
getRack call will re-check the cache.

We also protect against this possible NPE for safety

This is difficult to test directly but we should have an integration test to validate that PlacementPolicy is working as expected.

CC @eolivelli

Since the release of Pulsar 2.8 and upgrade to BK 4.12, the default rackAwarePlacementPolicy has been failing with the following exception: ``` 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 ``` This regression occured in commit 4c60262 The core of the issue is that `setConf` is called before `setBookieAddressResolver` has been set (see https://github.com/apache/bookkeeper/blob/034ef8566ad037937a4d58a28f70631175744f53/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L264-L286) This results in the NPE. We are safe to simply not eagerly init the cache in setConf as the getRack call will re-check the cache. We also protect against this possible NPE for safety

addisonj · 2021-09-20T01:40:57Z

/pulsarbot run-failure-checks

addisonj · 2021-09-20T04:50:16Z

/pulsarbot run-failure-checks

eolivelli

Good work Addison.

I wonder why we didn't catch this problem with tests.

Would you mind adding a reproducer test case for the problem?

If we cannot go with a unit test we can make a smoke integration test that simply configures rackaware bookie placement. No need to fully verify the behaviour but just that Pulsar does not break

eolivelli · 2021-09-20T05:11:08Z

...r-zookeeper-utils/src/main/java/org/apache/pulsar/zookeeper/ZkBookieRackAffinityMapping.java

-                            }
+                        BookieAddressResolver addressResolver = getBookieAddressResolver();
+                        if (addressResolver == null) {
+                            LOG.warn("Bookie address resolver not yet initialized, skipping resolution");


Does this case happen?
Or is this only a precautionary null check?

precautionary, I figure if it can return null we should be defensive

sijie · 2021-09-20T05:39:01Z

@eolivelli I believe the test set the DNS resolver manually before the test. Hence it doesn't catch the real usage.

eolivelli · 2021-09-20T05:42:27Z

I believe the test set the DNS resolver manually
Which test ?

My idea is to add an integration test that reproduces the behaviour of a sys admin that configures this feature.
It should be easy to see the error in the logs of the test.

It looks like we do not have such kind of integration tests.

@addisonj do you think it is worth do add it ?

btw please answer to my comment about the null check, and I am happy with this patch

addisonj · 2021-09-20T15:10:52Z

@eolivelli I do think it is worth the test, but I would suggest we make a follow up item to do that. While we could do a smoke test and see the logs, the log is pretty subtle and easy to miss... I would instead suggest we just put the time in to do a proper test that validates placement by querying metadata. However, I don't want to hold up this fix for that test as this is quite a big breakage

eolivelli · 2021-09-20T15:22:04Z

I don't want to hold up this fix for that test
This usually means that we won't be adding such test :-)
Usually as soon as the problem/urgency is fixed no one takes care of following up (there is no more interest in spending cycles).

By the way I don't want to block this patch, I hope you will find time to follow up.
Please create an issue

Since the release of Pulsar 2.8 and upgrade to BK 4.12, the default rackAwarePlacementPolicy has been failing with the following exception: ``` 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 ``` This regression occured in commit 4c60262 The core of the issue is that `setConf` is called before `setBookieAddressResolver` has been set (see https://github.com/apache/bookkeeper/blob/034ef8566ad037937a4d58a28f70631175744f53/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L264-L286) This results in the NPE. We are safe to simply not eagerly init the cache in setConf as the getRack call will re-check the cache. We also protect against this possible NPE for safety

Since the release of Pulsar 2.8 and upgrade to BK 4.12, the default rackAwarePlacementPolicy has been failing with the following exception: ``` 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 ``` This regression occured in commit apache@4c60262 The core of the issue is that `setConf` is called before `setBookieAddressResolver` has been set (see https://github.com/apache/bookkeeper/blob/034ef8566ad037937a4d58a28f70631175744f53/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L264-L286) This results in the NPE. We are safe to simply not eagerly init the cache in setConf as the getRack call will re-check the cache. We also protect against this possible NPE for safety (cherry picked from commit 9975fe4)

Since the release of Pulsar 2.8 and upgrade to BK 4.12, the default rackAwarePlacementPolicy has been failing with the following exception: ``` 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 ``` This regression occured in commit apache@4c60262 The core of the issue is that `setConf` is called before `setBookieAddressResolver` has been set (see https://github.com/apache/bookkeeper/blob/034ef8566ad037937a4d58a28f70631175744f53/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L264-L286) This results in the NPE. We are safe to simply not eagerly init the cache in setConf as the getRack call will re-check the cache. We also protect against this possible NPE for safety

addisonj requested review from sijie and eolivelli September 19, 2021 20:46

eolivelli requested changes Sep 20, 2021

View reviewed changes

sijie approved these changes Sep 20, 2021

View reviewed changes

eolivelli approved these changes Sep 20, 2021

View reviewed changes

eolivelli merged commit 9975fe4 into master Sep 20, 2021

eolivelli deleted the fix_placement_npe branch September 20, 2021 15:22

merlimat added this to the 2.9.0 milestone Sep 20, 2021

merlimat added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 20, 2021

merlimat assigned addisonj Sep 20, 2021

merlimat added release/2.8.2 type/bug The PR fixed a bug or issue reported a bug labels Sep 21, 2021

eolivelli added the release/2.9.0 label Oct 6, 2021

michaeljmarshall mentioned this pull request Jul 27, 2022

Fix rack awareness cache expiration data race #16825

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Fix rackaware placement policy init error #12097

Bugfix: Fix rackaware placement policy init error #12097

addisonj commented Sep 19, 2021

addisonj commented Sep 20, 2021

addisonj commented Sep 20, 2021

eolivelli left a comment

eolivelli Sep 20, 2021

addisonj Sep 20, 2021

sijie commented Sep 20, 2021

eolivelli commented Sep 20, 2021

addisonj commented Sep 20, 2021

eolivelli commented Sep 20, 2021

Bugfix: Fix rackaware placement policy init error #12097

Bugfix: Fix rackaware placement policy init error #12097

Conversation

addisonj commented Sep 19, 2021

addisonj commented Sep 20, 2021

addisonj commented Sep 20, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Sep 20, 2021

Choose a reason for hiding this comment

addisonj Sep 20, 2021

Choose a reason for hiding this comment

sijie commented Sep 20, 2021

eolivelli commented Sep 20, 2021

addisonj commented Sep 20, 2021

eolivelli commented Sep 20, 2021