Skip to content

Commit

Permalink
[fix][storage] Autorecovery default reppDnsResolverClass to ZkBookieR…
Browse files Browse the repository at this point in the history
…ackAffinityMapping (#15640)

* [Autorecovery] Default reppDnsResolverClass to ZkBookieRackAffinityMapping

* Improve documentation

Fixes: #18012

### Motivation

The current Bookkeeper configuration defaults to using `org.apache.bookkeeper.net.ScriptBasedMapping` for the `DNSToSwitchMapping` implementation. However, this default configuration does not align with the Broker's default configuration, which is `org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping`. As such, the default configuration for a Pulsar cluster does not lead to ideal rack awareness when ledgers need to be recovered. The result is that a user can configure a cluster for rack awareness and the brokers will honor that configuration, but the autorecovery process will not because it does not have the correct bookkeeper cluster topology view.

I propose we configure bookkeeper to use the broker's `ZkBookieRackAffinityMapping` class. That way, autorecovery will honor the operator's configured rack awareness policies out of the box.

### Modifications

* Add default value for `reppDnsResolverClass` to the `conf/bookkeeper.conf` configuration. This change effectively switches the default from `org.apache.bookkeeper.net.ScriptBasedMapping` to `org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping`.

### Verifying this change

I manually verified that the `ZkBookieRackAffinityMapping` works by running some tests in a minikube cluster deployed with the DataStax helm chart. I set up 3 racks, 4 bookies, and a topic with a E=2, Qw=2, and Qa=2. I then verified that the autorecovery pod correctly discovered the racks and then identified when an ensemble was not following the rack placement policy after two bookies were removed. I documented my testing a bit more here: datastax/pulsar-helm-chart#214.

### Does this pull request potentially affect one of the following parts:

It changes a default value. The tradeoff is that a user relying on the `ScriptBasedMapping` default might accidentally get switched to using the `ZkBookieRackAffinityMapping` implementation. Given that `ScriptBasedMapping` doesn't work out of the box, and that the broker's default to `ZkBookieRackAffinityMapping`, I think this is an acceptable tradeoff.

- [x] `doc`
  • Loading branch information
michaeljmarshall authored Oct 24, 2022
1 parent e913a10 commit 9812297
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions conf/bookkeeper.conf
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,17 @@ useV2WireProtocol=true
#
# ensemblePlacementPolicy=org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy

# The DNS resolver class used for resolving the network location of each bookie. All bookkeeper clients
# should be configured with the same value to ensure that each bookkeeper client builds
# the same network topology in order to then place ensembles consistently. The setting is used
# when using either RackawareEnsemblePlacementPolicy and RegionAwareEnsemblePlacementPolicy.
# The setting in this file is used to configure the bookkeeper client used in the bookkeeper, which
# is used by the autorecovery process.
# Some available options:
# - org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping (Pulsar default)
# - org.apache.bookkeeper.net.ScriptBasedMapping (Bookkeeper default)
reppDnsResolverClass=org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping

#############################################################################
## Netty server settings
#############################################################################
Expand Down

0 comments on commit 9812297

Please sign in to comment.