Skip to content

Commit 69deb1f

Browse files
michaeljmarshallmichaeljmarshall
authored
[fix][bookie] Correctly handle list configuration values (#17661)
* [fix][bookie] Correctly handle list configuration values * Remove unused import ### Motivation When the `metadataServiceUri` or the `zkServers` configuration for `BookieRackAffinityMapping` is provided as a comma delimited list, it is automatically parsed into an ArrayList by the configuration class because the Bookkeeper configuration class relies on the defaults in the `org.apache.commons.configuration.AbstractConfiguration` class. Here is a sample error: ``` 2022-07-29T19:25:43,437+0000 [main] ERROR org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl - Failed to initialize DNS Resolver org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping, used default subnet resolver java.lang.ClassCastException: class java.util.ArrayList cannot be cast to class java.lang.String (java.util.ArrayList and java.lang.String are in module java.base of loader 'bootstrap') ``` Also, see #6349 and #6343 for context. ### Modifications * Move the `castToString` method out to a shared class. * Use the `castToString` method to safely get the configuration value. ### Verifying this change This PR includes a test. ### Documentation - [x] `doc-not-needed`
1 parent 3208086 commit 69deb1f

File tree

4 files changed

+70
-23
lines changed

4 files changed

+70
-23
lines changed

‎pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public static MetadataStore createMetadataStore(Configuration conf) throws Metad
7777
store = (MetadataStore) storeProperty;
7878
} else {
7979
String url;
80-
String metadataServiceUri = (String) conf.getProperty("metadataServiceUri");
80+
String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataServiceUri"));
8181
if (StringUtils.isNotBlank(metadataServiceUri)) {
8282
try {
8383
url = metadataServiceUri.replaceFirst(METADATA_STORE_SCHEME + ":", "")
@@ -86,7 +86,7 @@ public static MetadataStore createMetadataStore(Configuration conf) throws Metad
8686
throw new MetadataException(Code.METADATA_SERVICE_ERROR, e);
8787
}
8888
} else {
89-
String zkServers = (String) conf.getProperty("zkServers");
89+
String zkServers = ConfigurationStringUtil.castToString(conf.getProperty("zkServers"));
9090
if (StringUtils.isBlank(zkServers)) {
9191
String errorMsg = String.format("Neither %s configuration set in the BK client configuration nor "
9292
+ "metadataServiceUri/zkServers set in bk server configuration", METADATA_STORE_INSTANCE);

‎pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/ConfigurationStringUtil.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.pulsar.bookie.rackawareness;
21+
22+
import java.util.ArrayList;
23+
import java.util.List;
24+
25+
public class ConfigurationStringUtil {
26+
27+
/**
28+
* The Bookkeeper configuration class converts comma delimited strings to ArrayLists, by default. Use
29+
* this method to ensure a configuration value is a {@link String}.
30+
* @param obj - object to convert to a string
31+
* @return The object's conversion to a string where Lists map to a comma delimited list.
32+
*/
33+
static String castToString(Object obj) {
34+
if (null == obj) {
35+
return "";
36+
}
37+
if (obj instanceof List<?>) {
38+
List<String> result = new ArrayList<>();
39+
for (Object o : (List<?>) obj) {
40+
result.add((String) o);
41+
}
42+
return String.join(",", result);
43+
} else {
44+
return obj.toString();
45+
}
46+
}
47+
48+
}

‎pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/IsolatedBookieEnsemblePlacementPolicy.java‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
import static org.apache.pulsar.bookie.rackawareness.BookieRackAffinityMapping.METADATA_STORE_INSTANCE;
2222
import io.netty.util.HashedWheelTimer;
23-
import java.util.ArrayList;
2423
import java.util.Arrays;
2524
import java.util.Collections;
2625
import java.util.HashSet;
@@ -80,7 +79,8 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
8079
Set<String> primaryIsolationGroups = new HashSet<>();
8180
Set<String> secondaryIsolationGroups = new HashSet<>();
8281
if (conf.getProperty(ISOLATION_BOOKIE_GROUPS) != null) {
83-
String isolationGroupsString = castToString(conf.getProperty(ISOLATION_BOOKIE_GROUPS));
82+
String isolationGroupsString = ConfigurationStringUtil
83+
.castToString(conf.getProperty(ISOLATION_BOOKIE_GROUPS));
8484
if (!isolationGroupsString.isEmpty()) {
8585
for (String isolationGroup : isolationGroupsString.split(",")) {
8686
primaryIsolationGroups.add(isolationGroup);
@@ -91,7 +91,8 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
9191
bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH).join();
9292
}
9393
if (conf.getProperty(SECONDARY_ISOLATION_BOOKIE_GROUPS) != null) {
94-
String secondaryIsolationGroupsString = castToString(conf.getProperty(SECONDARY_ISOLATION_BOOKIE_GROUPS));
94+
String secondaryIsolationGroupsString = ConfigurationStringUtil
95+
.castToString(conf.getProperty(SECONDARY_ISOLATION_BOOKIE_GROUPS));
9596
if (!secondaryIsolationGroupsString.isEmpty()) {
9697
for (String isolationGroup : secondaryIsolationGroupsString.split(",")) {
9798
secondaryIsolationGroups.add(isolationGroup);
@@ -102,21 +103,6 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
102103
return super.initialize(conf, optionalDnsResolver, timer, featureProvider, statsLogger, bookieAddressResolver);
103104
}
104105

105-
private static String castToString(Object obj) {
106-
if (null == obj) {
107-
return "";
108-
}
109-
if (obj instanceof List<?>) {
110-
List<String> result = new ArrayList<>();
111-
for (Object o : (List<?>) obj) {
112-
result.add((String) o);
113-
}
114-
return String.join(",", result);
115-
} else {
116-
return obj.toString();
117-
}
118-
}
119-
120106
@Override
121107
public PlacementResult<List<BookieId>> newEnsemble(int ensembleSize, int writeQuorumSize, int ackQuorumSize,
122108
Map<String, byte[]> customMetadata, Set<BookieId> excludeBookies)
@@ -181,9 +167,10 @@ private static Pair<Set<String>, Set<String>> getIsolationGroup(
181167
String className = IsolatedBookieEnsemblePlacementPolicy.class.getName();
182168
if (ensemblePlacementPolicyConfig.getPolicyClass().getName().equals(className)) {
183169
Map<String, Object> properties = ensemblePlacementPolicyConfig.getProperties();
184-
String primaryIsolationGroupString = castToString(properties.getOrDefault(ISOLATION_BOOKIE_GROUPS, ""));
185-
String secondaryIsolationGroupString =
186-
castToString(properties.getOrDefault(SECONDARY_ISOLATION_BOOKIE_GROUPS, ""));
170+
String primaryIsolationGroupString = ConfigurationStringUtil
171+
.castToString(properties.getOrDefault(ISOLATION_BOOKIE_GROUPS, ""));
172+
String secondaryIsolationGroupString = ConfigurationStringUtil
173+
.castToString(properties.getOrDefault(SECONDARY_ISOLATION_BOOKIE_GROUPS, ""));
187174
if (!primaryIsolationGroupString.isEmpty()) {
188175
pair.setLeft(new HashSet(Arrays.asList(primaryIsolationGroupString.split(","))));
189176
} else {

‎pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,18 @@ public void testBasic() throws Exception {
8282
assertNull(racks.get(2));
8383
}
8484

85+
@Test
86+
public void testMultipleMetadataServiceUris() {
87+
BookieRackAffinityMapping mapping1 = new BookieRackAffinityMapping();
88+
ClientConfiguration bkClientConf1 = new ClientConfiguration();
89+
bkClientConf1.setProperty("metadataServiceUri", "memory:local,memory:local");
90+
bkClientConf1.setProperty("zkTimeout", "100000");
91+
92+
mapping1.setBookieAddressResolver(BookieSocketAddress.LEGACY_BOOKIEID_RESOLVER);
93+
// This previously threw an exception when the metadataServiceUri was a comma delimited list.
94+
mapping1.setConf(bkClientConf1);
95+
}
96+
8597
@Test
8698
public void testInvalidRackName() {
8799
String data = "{\"group1\": {\"" + BOOKIE1

0 commit comments

Comments
 (0)