Skip to content

Commit 5d8619f

Browse files
committed
HBASE-28529 Use ZKClientConfig instead of system properties when setting zookeeper configurations (#5835)
Signed-off-by: Wellington Chevreuil <[email protected]> Reviewed-by: Andor Molnár <[email protected]> Reviewed-by: BukrosSzabolcs <[email protected]> (cherry picked from commit 6c6e776)
1 parent 46377c5 commit 5d8619f

File tree

6 files changed

+62
-108
lines changed

6 files changed

+62
-108
lines changed

hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ReadOnlyZKClient.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import org.apache.zookeeper.KeeperException;
3939
import org.apache.zookeeper.KeeperException.Code;
4040
import org.apache.zookeeper.ZooKeeper;
41+
import org.apache.zookeeper.client.ZKClientConfig;
4142
import org.apache.zookeeper.data.Stat;
4243
import org.slf4j.Logger;
4344
import org.slf4j.LoggerFactory;
@@ -75,6 +76,8 @@ public final class ReadOnlyZKClient implements Closeable {
7576

7677
private final int keepAliveTimeMs;
7778

79+
private final ZKClientConfig zkClientConfig;
80+
7881
private static abstract class Task implements Delayed {
7982

8083
protected long time = System.nanoTime();
@@ -136,10 +139,12 @@ public ReadOnlyZKClient(Configuration conf) {
136139
this.retryIntervalMs =
137140
conf.getInt(RECOVERY_RETRY_INTERVAL_MILLIS, DEFAULT_RECOVERY_RETRY_INTERVAL_MILLIS);
138141
this.keepAliveTimeMs = conf.getInt(KEEPALIVE_MILLIS, DEFAULT_KEEPALIVE_MILLIS);
142+
this.zkClientConfig = ZKConfig.getZKClientConfig(conf);
139143
LOG.debug(
140-
"Connect {} to {} with session timeout={}ms, retries {}, "
141-
+ "retry interval {}ms, keepAlive={}ms",
142-
getId(), connectString, sessionTimeoutMs, maxRetries, retryIntervalMs, keepAliveTimeMs);
144+
"Connect {} to {} with session timeout={}ms, retries={}, "
145+
+ "retry interval={}ms, keepAlive={}ms, zk client config={}",
146+
getId(), connectString, sessionTimeoutMs, maxRetries, retryIntervalMs, keepAliveTimeMs,
147+
zkClientConfig);
143148
Threads.setDaemonThreadRunning(new Thread(this::run),
144149
"ReadOnlyZKClient-" + connectString + "@" + getId());
145150
}
@@ -316,7 +321,7 @@ private ZooKeeper getZk() throws IOException {
316321
// may be closed when session expired
317322
if (zookeeper == null || !zookeeper.getState().isAlive()) {
318323
zookeeper = new ZooKeeper(connectString, sessionTimeoutMs, e -> {
319-
});
324+
}, zkClientConfig);
320325
}
321326
return zookeeper;
322327
}

hbase-common/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKConfig.java

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -26,19 +26,22 @@
2626
import org.apache.hadoop.hbase.HConstants;
2727
import org.apache.hadoop.util.StringUtils;
2828
import org.apache.yetus.audience.InterfaceAudience;
29+
import org.apache.zookeeper.client.ZKClientConfig;
2930

3031
import org.apache.hbase.thirdparty.com.google.common.base.Splitter;
3132

3233
/**
3334
* Utility methods for reading, and building the ZooKeeper configuration. The order and priority for
34-
* reading the config are as follows: (1). Property with "hbase.zookeeper.property." prefix from
35-
* HBase XML (2). other zookeeper related properties in HBASE XML
35+
* reading the config are as follows:
36+
* <ol>
37+
* <li>Property with "hbase.zookeeper.property." prefix from HBase XML.</li>
38+
* <li>other zookeeper related properties in HBASE XML</li>
39+
* </ol>
3640
*/
3741
@InterfaceAudience.Private
3842
public final class ZKConfig {
3943

4044
private static final String VARIABLE_START = "${";
41-
private static final String ZOOKEEPER_JAVA_PROPERTY_PREFIX = "zookeeper.";
4245

4346
private ZKConfig() {
4447
}
@@ -132,7 +135,6 @@ private static String getZKQuorumServersStringFromHbaseConfig(Configuration conf
132135
* @return Quorum servers
133136
*/
134137
public static String getZKQuorumServersString(Configuration conf) {
135-
setZooKeeperClientSystemProperties(HConstants.ZK_CFG_PROPERTY_PREFIX, conf);
136138
return getZKQuorumServersStringFromHbaseConfig(conf);
137139
}
138140

@@ -322,13 +324,19 @@ public String getZnodeParent() {
322324
}
323325
}
324326

327+
public static ZKClientConfig getZKClientConfig(Configuration conf) {
328+
Properties zkProperties = extractZKPropsFromHBaseConfig(conf);
329+
ZKClientConfig zkClientConfig = new ZKClientConfig();
330+
zkProperties.forEach((k, v) -> zkClientConfig.setProperty(k.toString(), v.toString()));
331+
return zkClientConfig;
332+
}
333+
325334
/**
326335
* Get the client ZK Quorum servers string
327336
* @param conf the configuration to read
328337
* @return Client quorum servers, or null if not specified
329338
*/
330339
public static String getClientZKQuorumServersString(Configuration conf) {
331-
setZooKeeperClientSystemProperties(HConstants.ZK_CFG_PROPERTY_PREFIX, conf);
332340
String clientQuromServers = conf.get(HConstants.CLIENT_ZOOKEEPER_QUORUM);
333341
if (clientQuromServers == null) {
334342
return null;
@@ -341,15 +349,4 @@ public static String getClientZKQuorumServersString(Configuration conf) {
341349
final String[] serverHosts = StringUtils.getStrings(clientQuromServers);
342350
return buildZKQuorumServerString(serverHosts, clientZkClientPort);
343351
}
344-
345-
private static void setZooKeeperClientSystemProperties(String prefix, Configuration conf) {
346-
Properties zkProperties = extractZKPropsFromHBaseConfig(conf);
347-
for (Entry<Object, Object> entry : zkProperties.entrySet()) {
348-
String key = entry.getKey().toString().trim();
349-
String value = entry.getValue().toString().trim();
350-
if (System.getProperty(ZOOKEEPER_JAVA_PROPERTY_PREFIX + key) == null) {
351-
System.setProperty(ZOOKEEPER_JAVA_PROPERTY_PREFIX + key, value);
352-
}
353-
}
354-
}
355352
}

hbase-common/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKConfig.java

Lines changed: 4 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.apache.hadoop.hbase.HConstants;
3030
import org.apache.hadoop.hbase.testclassification.MiscTests;
3131
import org.apache.hadoop.hbase.testclassification.SmallTests;
32+
import org.apache.zookeeper.client.ZKClientConfig;
3233
import org.junit.ClassRule;
3334
import org.junit.Test;
3435
import org.junit.experimental.categories.Category;
@@ -100,62 +101,22 @@ public void testClusterKeyWithMultiplePorts() throws Exception {
100101
}
101102

102103
@Test
103-
public void testZooKeeperTlsPropertiesClient() {
104+
public void testZooKeeperTlsProperties() {
104105
// Arrange
105106
Configuration conf = HBaseConfiguration.create();
106107
for (String p : ZOOKEEPER_CLIENT_TLS_PROPERTIES) {
107108
conf.set(HConstants.ZK_CFG_PROPERTY_PREFIX + p, p);
108-
String zkprop = "zookeeper." + p;
109-
System.clearProperty(zkprop);
110109
}
111110

112111
// Act
113-
ZKConfig.getClientZKQuorumServersString(conf);
112+
ZKClientConfig zkClientConfig = ZKConfig.getZKClientConfig(conf);
114113

115114
// Assert
116115
for (String p : ZOOKEEPER_CLIENT_TLS_PROPERTIES) {
117-
String zkprop = "zookeeper." + p;
118-
assertEquals("Invalid or unset system property: " + zkprop, p, System.getProperty(zkprop));
119-
System.clearProperty(zkprop);
116+
assertEquals("Invalid or unset system property: " + p, p, zkClientConfig.getProperty(p));
120117
}
121118
}
122119

123-
@Test
124-
public void testZooKeeperTlsPropertiesServer() {
125-
// Arrange
126-
Configuration conf = HBaseConfiguration.create();
127-
for (String p : ZOOKEEPER_CLIENT_TLS_PROPERTIES) {
128-
conf.set(HConstants.ZK_CFG_PROPERTY_PREFIX + p, p);
129-
String zkprop = "zookeeper." + p;
130-
System.clearProperty(zkprop);
131-
}
132-
133-
// Act
134-
ZKConfig.getZKQuorumServersString(conf);
135-
136-
// Assert
137-
for (String p : ZOOKEEPER_CLIENT_TLS_PROPERTIES) {
138-
String zkprop = "zookeeper." + p;
139-
assertEquals("Invalid or unset system property: " + zkprop, p, System.getProperty(zkprop));
140-
System.clearProperty(zkprop);
141-
}
142-
}
143-
144-
@Test
145-
public void testZooKeeperPropertiesDoesntOverwriteSystem() {
146-
// Arrange
147-
System.setProperty("zookeeper.a.b.c", "foo");
148-
Configuration conf = HBaseConfiguration.create();
149-
conf.set(HConstants.ZK_CFG_PROPERTY_PREFIX + "a.b.c", "bar");
150-
151-
// Act
152-
ZKConfig.getZKQuorumServersString(conf);
153-
154-
// Assert
155-
assertEquals("foo", System.getProperty("zookeeper.a.b.c"));
156-
System.clearProperty("zookeeper.a.b.c");
157-
}
158-
159120
private void testKey(String ensemble, int port, String znode) throws IOException {
160121
testKey(ensemble, port, znode, false); // not support multiple client ports
161122
}

hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/RecoverableZooKeeper.java

Lines changed: 33 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.apache.zookeeper.ZooDefs;
4242
import org.apache.zookeeper.ZooKeeper;
4343
import org.apache.zookeeper.ZooKeeper.States;
44+
import org.apache.zookeeper.client.ZKClientConfig;
4445
import org.apache.zookeeper.data.ACL;
4546
import org.apache.zookeeper.data.Stat;
4647
import org.apache.zookeeper.proto.CreateRequest;
@@ -49,19 +50,22 @@
4950
import org.slf4j.LoggerFactory;
5051

5152
/**
52-
* A zookeeper that can handle 'recoverable' errors. To handle recoverable errors, developers need
53-
* to realize that there are two classes of requests: idempotent and non-idempotent requests. Read
54-
* requests and unconditional sets and deletes are examples of idempotent requests, they can be
55-
* reissued with the same results. (Although, the delete may throw a NoNodeException on reissue its
56-
* effect on the ZooKeeper state is the same.) Non-idempotent requests need special handling,
57-
* application and library writers need to keep in mind that they may need to encode information in
58-
* the data or name of znodes to detect retries. A simple example is a create that uses a sequence
59-
* flag. If a process issues a create("/x-", ..., SEQUENCE) and gets a connection loss exception,
60-
* that process will reissue another create("/x-", ..., SEQUENCE) and get back x-111. When the
61-
* process does a getChildren("/"), it sees x-1,x-30,x-109,x-110,x-111, now it could be that x-109
62-
* was the result of the previous create, so the process actually owns both x-109 and x-111. An easy
63-
* way around this is to use "x-process id-" when doing the create. If the process is using an id of
64-
* 352, before reissuing the create it will do a getChildren("/") and see "x-222-1", "x-542-30",
53+
* A zookeeper that can handle 'recoverable' errors.
54+
* <p>
55+
* To handle recoverable errors, developers need to realize that there are two classes of requests:
56+
* idempotent and non-idempotent requests. Read requests and unconditional sets and deletes are
57+
* examples of idempotent requests, they can be reissued with the same results.
58+
* <p>
59+
* (Although, the delete may throw a NoNodeException on reissue its effect on the ZooKeeper state is
60+
* the same.) Non-idempotent requests need special handling, application and library writers need to
61+
* keep in mind that they may need to encode information in the data or name of znodes to detect
62+
* retries. A simple example is a create that uses a sequence flag. If a process issues a
63+
* create("/x-", ..., SEQUENCE) and gets a connection loss exception, that process will reissue
64+
* another create("/x-", ..., SEQUENCE) and get back x-111. When the process does a
65+
* getChildren("/"), it sees x-1,x-30,x-109,x-110,x-111, now it could be that x-109 was the result
66+
* of the previous create, so the process actually owns both x-109 and x-111. An easy way around
67+
* this is to use "x-process id-" when doing the create. If the process is using an id of 352,
68+
* before reissuing the create it will do a getChildren("/") and see "x-222-1", "x-542-30",
6569
* "x-352-109", x-333-110". The process will know that the original create succeeded an the znode it
6670
* created is "x-352-109".
6771
* @see "https://cwiki.apache.org/confluence/display/HADOOP2/ZooKeeper+ErrorHandling"
@@ -79,37 +83,31 @@ public class RecoverableZooKeeper {
7983
private final int sessionTimeout;
8084
private final String quorumServers;
8185
private final int maxMultiSize;
86+
private final ZKClientConfig zkClientConfig;
8287

8388
/**
84-
* See {@link #connect(Configuration, String, Watcher, String)}
89+
* See {@link #connect(Configuration, String, Watcher, String, ZKClientConfig)}.
8590
*/
8691
public static RecoverableZooKeeper connect(Configuration conf, Watcher watcher)
8792
throws IOException {
8893
String ensemble = ZKConfig.getZKQuorumServersString(conf);
89-
return connect(conf, ensemble, watcher);
90-
}
91-
92-
/**
93-
* See {@link #connect(Configuration, String, Watcher, String)}
94-
*/
95-
public static RecoverableZooKeeper connect(Configuration conf, String ensemble, Watcher watcher)
96-
throws IOException {
97-
return connect(conf, ensemble, watcher, null);
94+
return connect(conf, ensemble, watcher, null, null);
9895
}
9996

10097
/**
10198
* Creates a new connection to ZooKeeper, pulling settings and ensemble config from the specified
10299
* configuration object using methods from {@link ZKConfig}. Sets the connection status monitoring
103100
* watcher to the specified watcher.
104-
* @param conf configuration to pull ensemble and other settings from
105-
* @param watcher watcher to monitor connection changes
106-
* @param ensemble ZooKeeper servers quorum string
107-
* @param identifier value used to identify this client instance.
101+
* @param conf configuration to pull ensemble and other settings from
102+
* @param watcher watcher to monitor connection changes
103+
* @param ensemble ZooKeeper servers quorum string
104+
* @param identifier value used to identify this client instance.
105+
* @param zkClientConfig client specific configurations for this instance
108106
* @return connection to zookeeper
109107
* @throws IOException if unable to connect to zk or config problem
110108
*/
111109
public static RecoverableZooKeeper connect(Configuration conf, String ensemble, Watcher watcher,
112-
final String identifier) throws IOException {
110+
final String identifier, ZKClientConfig zkClientConfig) throws IOException {
113111
if (ensemble == null) {
114112
throw new IOException("Unable to determine ZooKeeper ensemble");
115113
}
@@ -122,14 +120,12 @@ public static RecoverableZooKeeper connect(Configuration conf, String ensemble,
122120
int maxSleepTime = conf.getInt("zookeeper.recovery.retry.maxsleeptime", 60000);
123121
int multiMaxSize = conf.getInt("zookeeper.multi.max.size", 1024 * 1024);
124122
return new RecoverableZooKeeper(ensemble, timeout, watcher, retry, retryIntervalMillis,
125-
maxSleepTime, identifier, multiMaxSize);
123+
maxSleepTime, identifier, multiMaxSize, zkClientConfig);
126124
}
127125

128-
@edu.umd.cs.findbugs.annotations.SuppressWarnings(value = "DE_MIGHT_IGNORE",
129-
justification = "None. Its always been this way.")
130-
public RecoverableZooKeeper(String quorumServers, int sessionTimeout, Watcher watcher,
131-
int maxRetries, int retryIntervalMillis, int maxSleepTime, String identifier, int maxMultiSize)
132-
throws IOException {
126+
RecoverableZooKeeper(String quorumServers, int sessionTimeout, Watcher watcher, int maxRetries,
127+
int retryIntervalMillis, int maxSleepTime, String identifier, int maxMultiSize,
128+
ZKClientConfig zkClientConfig) throws IOException {
133129
// TODO: Add support for zk 'chroot'; we don't add it to the quorumServers String as we should.
134130
this.retryCounterFactory =
135131
new RetryCounterFactory(maxRetries + 1, retryIntervalMillis, maxSleepTime);
@@ -147,12 +143,7 @@ public RecoverableZooKeeper(String quorumServers, int sessionTimeout, Watcher wa
147143
this.sessionTimeout = sessionTimeout;
148144
this.quorumServers = quorumServers;
149145
this.maxMultiSize = maxMultiSize;
150-
151-
try {
152-
checkZk();
153-
} catch (Exception x) {
154-
/* ignore */
155-
}
146+
this.zkClientConfig = zkClientConfig;
156147
}
157148

158149
/**
@@ -171,10 +162,10 @@ public int getMaxMultiSizeLimit() {
171162
* @return The created ZooKeeper connection object
172163
* @throws KeeperException if a ZooKeeper operation fails
173164
*/
174-
protected synchronized ZooKeeper checkZk() throws KeeperException {
165+
private synchronized ZooKeeper checkZk() throws KeeperException {
175166
if (this.zk == null) {
176167
try {
177-
this.zk = new ZooKeeper(quorumServers, sessionTimeout, watcher);
168+
this.zk = new ZooKeeper(quorumServers, sessionTimeout, watcher, zkClientConfig);
178169
} catch (IOException ex) {
179170
LOG.warn("Unable to create ZooKeeper Connection", ex);
180171
throw new KeeperException.OperationTimeoutException();

hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,8 @@ public ZKWatcher(Configuration conf, String identifier, Abortable abortable,
176176
this.abortable = abortable;
177177
this.znodePaths = new ZNodePaths(conf);
178178
PendingWatcher pendingWatcher = new PendingWatcher();
179-
this.recoverableZooKeeper =
180-
RecoverableZooKeeper.connect(conf, quorum, pendingWatcher, identifier);
179+
this.recoverableZooKeeper = RecoverableZooKeeper.connect(conf, quorum, pendingWatcher,
180+
identifier, ZKConfig.getZKClientConfig(conf));
181181
pendingWatcher.prepare(this);
182182
if (canCreateBaseZNode) {
183183
try {

hbase-zookeeper/src/test/java/org/apache/hadoop/hbase/zookeeper/TestRecoverableZooKeeper.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public void testSetDataVersionMismatchInLoop() throws Exception {
7676
Configuration conf = TEST_UTIL.getConfiguration();
7777
ZKWatcher zkw = new ZKWatcher(conf, "testSetDataVersionMismatchInLoop", abortable, true);
7878
String ensemble = ZKConfig.getZKQuorumServersString(conf);
79-
RecoverableZooKeeper rzk = RecoverableZooKeeper.connect(conf, ensemble, zkw);
79+
RecoverableZooKeeper rzk = RecoverableZooKeeper.connect(conf, ensemble, zkw, null, null);
8080
rzk.create(znode, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
8181
rzk.setData(znode, Bytes.toBytes("OPENING"), 0);
8282
Field zkField = RecoverableZooKeeper.class.getDeclaredField("zk");

0 commit comments

Comments
 (0)