You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by ra...@apache.org on 2020/02/12 22:07:53 UTC
[curator] branch master updated: CURATOR-551 (#345)
This is an automated email from the ASF dual-hosted git repository.
randgalt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/curator.git
The following commit(s) were added to refs/heads/master by this push:
new 75226fb CURATOR-551 (#345)
75226fb is described below
commit 75226fbf211eff6c1f7ecfceadd1f996a0515f83
Author: Jordan Zimmerman <jo...@jordanzimmerman.com>
AuthorDate: Wed Feb 12 17:07:45 2020 -0500
CURATOR-551 (#345)
Commit 26364c6186fc7c09a9462557b1ca791e9aa70006 (Sat Sep 26 13:13:02 2015) changed HandleHolder.getNewConnectionString() was changed to return the new connection string instead of just a boolean. I believe the value returned should have been ensembleProvider.getConnectionString() not helper.getConnectionString(). TBH I no longer remember the genesis of this change but I can't make the current implementation make sense.
Additionally, a change was made to optionally call zooKeeper.updateServerList(). When this path is taken the handle holder's connection needs to be updated as well or we'll get an infinite loop of changes. The path that runs when ensembleProvider.updateServerListEnabled() is false ends up setting handle holder's connection to ensembleProvider.getConnectionString().
I'm loathe to make such low level changes in code that's existed for a long time. But, my investigation shows that this is how it should be. Hopefully, users can do testing.
---
.../java/org/apache/curator/ConnectionState.java | 25 ++++----
.../main/java/org/apache/curator/HandleHolder.java | 70 ++++++----------------
.../src/main/java/org/apache/curator/Helper.java | 57 ++++++++++++++++++
3 files changed, 89 insertions(+), 63 deletions(-)
diff --git a/curator-client/src/main/java/org/apache/curator/ConnectionState.java b/curator-client/src/main/java/org/apache/curator/ConnectionState.java
index 24b9948..726b418 100644
--- a/curator-client/src/main/java/org/apache/curator/ConnectionState.java
+++ b/curator-client/src/main/java/org/apache/curator/ConnectionState.java
@@ -48,7 +48,7 @@ class ConnectionState implements Watcher, Closeable
private static final int MAX_BACKGROUND_EXCEPTIONS = 10;
private static final boolean LOG_EVENTS = Boolean.getBoolean(DebugUtils.PROPERTY_LOG_EVENTS);
private static final Logger log = LoggerFactory.getLogger(ConnectionState.class);
- private final HandleHolder zooKeeper;
+ private final HandleHolder handleHolder;
private final AtomicBoolean isConnected = new AtomicBoolean(false);
private final AtomicInteger lastNegotiatedSessionTimeoutMs = new AtomicInteger(0);
private final EnsembleProvider ensembleProvider;
@@ -73,7 +73,7 @@ class ConnectionState implements Watcher, Closeable
parentWatchers.offer(parentWatcher);
}
- zooKeeper = new HandleHolder(zookeeperFactory, this, ensembleProvider, sessionTimeoutMs, canBeReadOnly);
+ handleHolder = new HandleHolder(zookeeperFactory, this, ensembleProvider, sessionTimeoutMs, canBeReadOnly);
}
ZooKeeper getZooKeeper() throws Exception
@@ -96,7 +96,7 @@ class ConnectionState implements Watcher, Closeable
checkTimeouts();
}
- return zooKeeper.getZooKeeper();
+ return handleHolder.getZooKeeper();
}
boolean isConnected()
@@ -122,7 +122,7 @@ class ConnectionState implements Watcher, Closeable
CloseableUtils.closeQuietly(ensembleProvider);
try
{
- zooKeeper.closeAndClear(waitForShutdownTimeoutMs);
+ handleHolder.closeAndClear(waitForShutdownTimeoutMs);
}
catch ( Exception e )
{
@@ -173,7 +173,7 @@ class ConnectionState implements Watcher, Closeable
connectionStartMs = System.currentTimeMillis();
if ( newIsConnected )
{
- lastNegotiatedSessionTimeoutMs.set(zooKeeper.getNegotiatedSessionTimeoutMs());
+ lastNegotiatedSessionTimeoutMs.set(handleHolder.getNegotiatedSessionTimeoutMs());
log.debug("Negotiated session timeout: " + lastNegotiatedSessionTimeoutMs.get());
}
}
@@ -200,8 +200,8 @@ class ConnectionState implements Watcher, Closeable
isConnected.set(false);
connectionStartMs = System.currentTimeMillis();
- zooKeeper.closeAndReset();
- zooKeeper.getZooKeeper(); // initiate connection
+ handleHolder.closeAndReset();
+ handleHolder.getZooKeeper(); // initiate connection
}
private synchronized void checkTimeouts() throws Exception
@@ -212,7 +212,7 @@ class ConnectionState implements Watcher, Closeable
@Override
public String call()
{
- newConnectionString.set(zooKeeper.getNewConnectionString());
+ newConnectionString.set(handleHolder.getNewConnectionString());
return newConnectionString.get();
}
};
@@ -251,7 +251,7 @@ class ConnectionState implements Watcher, Closeable
if ( !Boolean.getBoolean(DebugUtils.PROPERTY_DONT_LOG_CONNECTION_ISSUES) )
{
long elapsed = System.currentTimeMillis() - connectionStartMs;
- log.error(String.format("Connection timed out for connection string (%s) and timeout (%d) / elapsed (%d)", zooKeeper.getConnectionString(), connectionTimeoutMs, elapsed), connectionLossException);
+ log.error(String.format("Connection timed out for connection string (%s) and timeout (%d) / elapsed (%d)", handleHolder.getConnectionString(), connectionTimeoutMs, elapsed), connectionLossException);
}
new EventTrace("connections-timed-out", tracer.get(), getSessionId()).commit();
throw connectionLossException;
@@ -271,7 +271,7 @@ class ConnectionState implements Watcher, Closeable
public long getSessionId() {
long sessionId = 0;
try {
- ZooKeeper zk = zooKeeper.getZooKeeper();
+ ZooKeeper zk = handleHolder.getZooKeeper();
if (zk != null) {
sessionId = zk.getSessionId();
}
@@ -329,7 +329,7 @@ class ConnectionState implements Watcher, Closeable
if ( checkNewConnectionString )
{
- String newConnectionString = zooKeeper.getNewConnectionString();
+ String newConnectionString = handleHolder.getNewConnectionString();
if ( newConnectionString != null )
{
handleNewConnectionString(newConnectionString);
@@ -346,7 +346,7 @@ class ConnectionState implements Watcher, Closeable
try
{
- ZooKeeper zooKeeper = this.zooKeeper.getZooKeeper();
+ ZooKeeper zooKeeper = handleHolder.getZooKeeper();
if ( zooKeeper == null )
{
log.warn("Could not update the connection string because getZooKeeper() returned null.");
@@ -356,6 +356,7 @@ class ConnectionState implements Watcher, Closeable
if ( ensembleProvider.updateServerListEnabled() )
{
zooKeeper.updateServerList(newConnectionString);
+ handleHolder.resetConnectionString(newConnectionString);
}
else
{
diff --git a/curator-client/src/main/java/org/apache/curator/HandleHolder.java b/curator-client/src/main/java/org/apache/curator/HandleHolder.java
index 29c3962..e87cf4a 100644
--- a/curator-client/src/main/java/org/apache/curator/HandleHolder.java
+++ b/curator-client/src/main/java/org/apache/curator/HandleHolder.java
@@ -34,15 +34,6 @@ class HandleHolder
private volatile Helper helper;
- private interface Helper
- {
- ZooKeeper getZooKeeper() throws Exception;
-
- String getConnectionString();
-
- int getNegotiatedSessionTimeoutMs();
- }
-
HandleHolder(ZookeeperFactory zookeeperFactory, Watcher watcher, EnsembleProvider ensembleProvider, int sessionTimeout, boolean canBeReadOnly)
{
this.zookeeperFactory = zookeeperFactory;
@@ -70,7 +61,16 @@ class HandleHolder
String getNewConnectionString()
{
String helperConnectionString = (helper != null) ? helper.getConnectionString() : null;
- return ((helperConnectionString != null) && !ensembleProvider.getConnectionString().equals(helperConnectionString)) ? helperConnectionString : null;
+ String ensembleProviderConnectionString = ensembleProvider.getConnectionString();
+ return ((helperConnectionString != null) && !ensembleProviderConnectionString.equals(helperConnectionString)) ? ensembleProviderConnectionString : null;
+ }
+
+ void resetConnectionString(String connectionString)
+ {
+ if ( helper != null )
+ {
+ helper.resetConnectionString(connectionString);
+ }
}
void closeAndClear(int waitForShutdownTimeoutMs) throws Exception
@@ -83,60 +83,28 @@ class HandleHolder
{
internalClose(0);
+ Helper.Data data = new Helper.Data(); // data shared between initial Helper and the un-synchronized Helper
// first helper is synchronized when getZooKeeper is called. Subsequent calls
// are not synchronized.
- helper = new Helper()
+ //noinspection NonAtomicOperationOnVolatileField
+ helper = new Helper(data)
{
- private volatile ZooKeeper zooKeeperHandle = null;
- private volatile String connectionString = null;
-
@Override
- public ZooKeeper getZooKeeper() throws Exception
+ ZooKeeper getZooKeeper() throws Exception
{
synchronized(this)
{
- if ( zooKeeperHandle == null )
+ if ( data.zooKeeperHandle == null )
{
- connectionString = ensembleProvider.getConnectionString();
- zooKeeperHandle = zookeeperFactory.newZooKeeper(connectionString, sessionTimeout, watcher, canBeReadOnly);
+ resetConnectionString(ensembleProvider.getConnectionString());
+ data.zooKeeperHandle = zookeeperFactory.newZooKeeper(data.connectionString, sessionTimeout, watcher, canBeReadOnly);
}
- helper = new Helper()
- {
- @Override
- public ZooKeeper getZooKeeper() throws Exception
- {
- return zooKeeperHandle;
- }
-
- @Override
- public String getConnectionString()
- {
- return connectionString;
- }
-
- @Override
- public int getNegotiatedSessionTimeoutMs()
- {
- return (zooKeeperHandle != null) ? zooKeeperHandle.getSessionTimeout() : 0;
- }
- };
+ helper = new Helper(data);
- return zooKeeperHandle;
+ return super.getZooKeeper();
}
}
-
- @Override
- public String getConnectionString()
- {
- return connectionString;
- }
-
- @Override
- public int getNegotiatedSessionTimeoutMs()
- {
- return (zooKeeperHandle != null) ? zooKeeperHandle.getSessionTimeout() : 0;
- }
};
}
diff --git a/curator-client/src/main/java/org/apache/curator/Helper.java b/curator-client/src/main/java/org/apache/curator/Helper.java
new file mode 100644
index 0000000..d38c47e
--- /dev/null
+++ b/curator-client/src/main/java/org/apache/curator/Helper.java
@@ -0,0 +1,57 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator;
+
+import org.apache.zookeeper.ZooKeeper;
+
+class Helper
+{
+ private final Data data;
+
+ static class Data
+ {
+ volatile ZooKeeper zooKeeperHandle = null;
+ volatile String connectionString = null;
+ }
+
+ Helper(Data data)
+ {
+ this.data = data;
+ }
+
+ ZooKeeper getZooKeeper() throws Exception
+ {
+ return data.zooKeeperHandle;
+ }
+
+ String getConnectionString()
+ {
+ return data.connectionString;
+ }
+
+ int getNegotiatedSessionTimeoutMs()
+ {
+ return (data.zooKeeperHandle != null) ? data.zooKeeperHandle.getSessionTimeout() : 0;
+ }
+
+ void resetConnectionString(String connectionString)
+ {
+ data.connectionString = connectionString;
+ }
+}