Skip to content

Commit 28501d8

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 4d7ce1a commit 28501d8

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
@@ -25,19 +25,22 @@
2525
import org.apache.hadoop.hbase.HConstants;
2626
import org.apache.hadoop.util.StringUtils;
2727
import org.apache.yetus.audience.InterfaceAudience;
28+
import org.apache.zookeeper.client.ZKClientConfig;
2829

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

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

3943
private static final String VARIABLE_START = "${";
40-
private static final String ZOOKEEPER_JAVA_PROPERTY_PREFIX = "zookeeper.";
4144

4245
private ZKConfig() {
4346
}
@@ -131,7 +134,6 @@ private static String getZKQuorumServersStringFromHbaseConfig(Configuration conf
131134
* @return Quorum servers
132135
*/
133136
public static String getZKQuorumServersString(Configuration conf) {
134-
setZooKeeperClientSystemProperties(HConstants.ZK_CFG_PROPERTY_PREFIX, conf);
135137
return getZKQuorumServersStringFromHbaseConfig(conf);
136138
}
137139

@@ -299,13 +301,19 @@ public String getZnodeParent() {
299301
}
300302
}
301303

304+
public static ZKClientConfig getZKClientConfig(Configuration conf) {
305+
Properties zkProperties = extractZKPropsFromHBaseConfig(conf);
306+
ZKClientConfig zkClientConfig = new ZKClientConfig();
307+
zkProperties.forEach((k, v) -> zkClientConfig.setProperty(k.toString(), v.toString()));
308+
return zkClientConfig;
309+
}
310+
302311
/**
303312
* Get the client ZK Quorum servers string
304313
* @param conf the configuration to read
305314
* @return Client quorum servers, or null if not specified
306315
*/
307316
public static String getClientZKQuorumServersString(Configuration conf) {
308-
setZooKeeperClientSystemProperties(HConstants.ZK_CFG_PROPERTY_PREFIX, conf);
309317
String clientQuromServers = conf.get(HConstants.CLIENT_ZOOKEEPER_QUORUM);
310318
if (clientQuromServers == null) {
311319
return null;
@@ -318,15 +326,4 @@ public static String getClientZKQuorumServersString(Configuration conf) {
318326
final String[] serverHosts = StringUtils.getStrings(clientQuromServers);
319327
return buildZKQuorumServerString(serverHosts, clientZkClientPort);
320328
}
321-
322-
private static void setZooKeeperClientSystemProperties(String prefix, Configuration conf) {
323-
Properties zkProperties = extractZKPropsFromHBaseConfig(conf);
324-
for (Entry<Object, Object> entry : zkProperties.entrySet()) {
325-
String key = entry.getKey().toString().trim();
326-
String value = entry.getValue().toString().trim();
327-
if (System.getProperty(ZOOKEEPER_JAVA_PROPERTY_PREFIX + key) == null) {
328-
System.setProperty(ZOOKEEPER_JAVA_PROPERTY_PREFIX + key, value);
329-
}
330-
}
331-
}
332329
}

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
@@ -42,6 +42,7 @@
4242
import org.apache.zookeeper.ZooDefs;
4343
import org.apache.zookeeper.ZooKeeper;
4444
import org.apache.zookeeper.ZooKeeper.States;
45+
import org.apache.zookeeper.client.ZKClientConfig;
4546
import org.apache.zookeeper.data.ACL;
4647
import org.apache.zookeeper.data.Stat;
4748
import org.apache.zookeeper.proto.CreateRequest;
@@ -50,19 +51,22 @@
5051
import org.slf4j.LoggerFactory;
5152

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

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

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

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

159150
/**
@@ -172,10 +163,10 @@ public int getMaxMultiSizeLimit() {
172163
* @return The created ZooKeeper connection object
173164
* @throws KeeperException if a ZooKeeper operation fails
174165
*/
175-
protected synchronized ZooKeeper checkZk() throws KeeperException {
166+
private synchronized ZooKeeper checkZk() throws KeeperException {
176167
if (this.zk == null) {
177168
try {
178-
this.zk = new ZooKeeper(quorumServers, sessionTimeout, watcher);
169+
this.zk = new ZooKeeper(quorumServers, sessionTimeout, watcher, zkClientConfig);
179170
} catch (IOException ex) {
180171
LOG.warn("Unable to create ZooKeeper Connection", ex);
181172
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, "OPENING".getBytes(), 0);
8282
Field zkField = RecoverableZooKeeper.class.getDeclaredField("zk");

0 commit comments

Comments
 (0)