Also, Pat, the JIRA is still open. I will look to revert in a minute although I think the sync changes may necessitate a new JIRA entirely.
On Wed, Dec 28, 2011 at 1:31 PM, Camille Fournier <[email protected]> wrote: > After looking for a few mins, here are my observations: > > The implementation of HashSet in jdk 1.6 uses an underlying HashMap, which > uses a bog-standard int for the size. So, I completely agree that we can > get invalid results for a point in time. But does anyone really care about > the *exact* moment-in-time number of cnxns in the system, in a system where > cnxns are coming and going? At best you'll see a value that will likely be > out of date the moment you read it. > > But, if we're going to do this, I do think there may be a case for > refactoring to a concurrent data structure given that we are randomly > syncing on the cnxns set in a way that makes adding additional interactions > with it error prone in this way. And we should definitely remove that > getConnections method, if anyone ever iterated over that set they would be > in a sad concurrency situation. > Seems possibly worth a whole new ticket to do this. What do you think? > > Thanks, > C > > On Wed, Dec 28, 2011 at 1:12 PM, Camille Fournier <[email protected]>wrote: > >> I'm not sure it's meaningful enough to be worth the sync overhead. We >> should look into that. The alternative if we must is using a proper >> concurrent collection. Neha, any thoughts? >> >> C >> On Dec 28, 2011 12:45 PM, "Patrick Hunt" <[email protected]> wrote: >> >>> I believe there is a bug in this commit. The "cnxns" size() call is >>> not being synchronized. This will lead to invalid results at best, at >>> worst outright failure (hard to say w/o knowing the implementation of >>> HashSet). >>> >>> Camille can you work with Neha to get this fixed? Perhaps in the >>> meantime (if it's going to take a while) you can revert this change, >>> re-open the jira, update the patch, and reapply at some later time? >>> >>> Patrick >>> >>> On Wed, Dec 28, 2011 at 6:55 AM, <[email protected]> wrote: >>> > Author: camille >>> > Date: Wed Dec 28 14:55:37 2011 >>> > New Revision: 1225200 >>> > >>> > URL: http://svn.apache.org/viewvc?rev=1225200&view=rev >>> > Log: >>> > ZOOKEEPER-1321: Add number of client connections metric in JMX and srvr >>> (Neha Narkhede via camille) >>> > >>> > Modified: >>> > zookeeper/trunk/CHANGES.txt >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerStats.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java >>> > >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMXBean.java >>> > >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java >>> > >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java >>> > >>> > Modified: zookeeper/trunk/CHANGES.txt >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/CHANGES.txt?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- zookeeper/trunk/CHANGES.txt (original) >>> > +++ zookeeper/trunk/CHANGES.txt Wed Dec 28 14:55:37 2011 >>> > @@ -162,6 +162,8 @@ IMPROVEMENTS: >>> > >>> > ZOOKEEPER-1342. quorum Listener & LearnerCnxAcceptor are missing >>> > thread names (Rakesh R via phunt) >>> > + >>> > + ZOOKEEPER-1321. Add number of client connections metric in JMX and >>> srvr (Neha Narkhede via camille) >>> > >>> > Release 3.4.0 - >>> > >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -749,7 +749,8 @@ public class NIOServerCnxn extends Serve >>> > >>> > print("packets_received", stats.getPacketsReceived()); >>> > print("packets_sent", stats.getPacketsSent()); >>> > - >>> > + print("num_alive_connections", >>> stats.getNumAliveClientConnections()); >>> > + >>> > print("outstanding_requests", >>> stats.getOutstandingRequests()); >>> > >>> > print("server_state", stats.getServerState()); >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -32,14 +32,14 @@ import java.util.HashMap; >>> > import java.util.HashSet; >>> > import java.util.Set; >>> > >>> > +import javax.security.auth.login.Configuration; >>> > +import javax.security.auth.login.LoginException; >>> > + >>> > import org.apache.zookeeper.Login; >>> > import org.apache.zookeeper.server.auth.SaslServerCallbackHandler; >>> > import org.slf4j.Logger; >>> > import org.slf4j.LoggerFactory; >>> > >>> > -import javax.security.auth.login.Configuration; >>> > -import javax.security.auth.login.LoginException; >>> > - >>> > public class NIOServerCnxnFactory extends ServerCnxnFactory implements >>> Runnable { >>> > private static final Logger LOG = >>> LoggerFactory.getLogger(NIOServerCnxnFactory.class); >>> > >>> > @@ -78,7 +78,6 @@ public class NIOServerCnxnFactory extend >>> > >>> > int maxClientCnxns = 60; >>> > >>> > - >>> > /** >>> > * Construct a new server connection factory which will accept an >>> unlimited number >>> > * of concurrent connections from each client (up to the file >>> descriptor >>> > @@ -122,7 +121,7 @@ public class NIOServerCnxnFactory extend >>> > public void setMaxClientCnxnsPerHost(int max) { >>> > maxClientCnxns = max; >>> > } >>> > - >>> > + >>> > @Override >>> > public void start() { >>> > // ensure thread is started once and only once >>> > @@ -187,7 +186,7 @@ public class NIOServerCnxnFactory extend >>> > return s.size(); >>> > } >>> > } >>> > - >>> > + >>> > public void run() { >>> > while (!ss.socket().isClosed()) { >>> > try { >>> > @@ -323,4 +322,8 @@ public class NIOServerCnxnFactory extend >>> > return cnxns; >>> > } >>> > >>> > + @Override >>> > + public int getNumAliveConnections() { >>> > + return cnxns.size(); >>> > + } >>> > } >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -560,7 +560,8 @@ public class NettyServerCnxn extends Ser >>> > >>> > print("packets_received", stats.getPacketsReceived()); >>> > print("packets_sent", stats.getPacketsSent()); >>> > - >>> > + print("num_alive_connections", >>> stats.getNumAliveClientConnections()); >>> > + >>> > print("outstanding_requests", >>> stats.getOutstandingRequests()); >>> > >>> > print("server_state", stats.getServerState()); >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -411,5 +411,10 @@ public class NettyServerCnxnFactory exte >>> > } >>> > } >>> > } >>> > + >>> > + @Override >>> > + public int getNumAliveConnections() { >>> > + return cnxns.size(); >>> > + } >>> > >>> > } >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -24,15 +24,16 @@ import java.nio.ByteBuffer; >>> > import java.util.HashMap; >>> > >>> > import javax.management.JMException; >>> > -import org.slf4j.Logger; >>> > -import org.slf4j.LoggerFactory; >>> > -import org.apache.zookeeper.jmx.MBeanRegistry; >>> > + >>> > import org.apache.zookeeper.Login; >>> > +import org.apache.zookeeper.jmx.MBeanRegistry; >>> > import org.apache.zookeeper.server.auth.SaslServerCallbackHandler; >>> > +import org.slf4j.Logger; >>> > +import org.slf4j.LoggerFactory; >>> > >>> > public abstract class ServerCnxnFactory { >>> > >>> > - public static final String ZOOKEEPER_SERVER_CNXN_FACTORY = >>> "zookeeper.serverCnxnFactory"; >>> > + public static final String ZOOKEEPER_SERVER_CNXN_FACTORY = >>> "zookeeper.serverCnxnFactory"; >>> > >>> > public interface PacketProcessor { >>> > public void processPacket(ByteBuffer packet, ServerCnxn src); >>> > @@ -49,6 +50,8 @@ public abstract class ServerCnxnFactory >>> > >>> > public abstract Iterable<ServerCnxn> getConnections(); >>> > >>> > + public abstract int getNumAliveConnections(); >>> > + >>> > public abstract void closeSession(long sessionId); >>> > >>> > public abstract void configure(InetSocketAddress addr, >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerStats.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerStats.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerStats.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerStats.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -19,6 +19,7 @@ >>> > package org.apache.zookeeper.server; >>> > >>> > >>> > + >>> > /** >>> > * Basic Server Statistics >>> > */ >>> > @@ -29,13 +30,14 @@ public class ServerStats { >>> > private long minLatency = Long.MAX_VALUE; >>> > private long totalLatency = 0; >>> > private long count = 0; >>> > - >>> > + >>> > private final Provider provider; >>> > >>> > public interface Provider { >>> > public long getOutstandingRequests(); >>> > public long getLastProcessedZxid(); >>> > public String getState(); >>> > + public int getNumAliveConnections(); >>> > } >>> > >>> > public ServerStats(Provider provider) { >>> > @@ -75,9 +77,14 @@ public class ServerStats { >>> > } >>> > >>> > public String getServerState() { >>> > - return provider.getState(); >>> > + return provider.getState(); >>> > } >>> > - >>> > + >>> > + /** The number of client connections alive to this server */ >>> > + public int getNumAliveClientConnections() { >>> > + return provider.getNumAliveConnections(); >>> > + } >>> > + >>> > @Override >>> > public String toString(){ >>> > StringBuilder sb = new StringBuilder(); >>> > @@ -85,6 +92,8 @@ public class ServerStats { >>> > + getAvgLatency() + "/" + getMaxLatency() + "\n"); >>> > sb.append("Received: " + getPacketsReceived() + "\n"); >>> > sb.append("Sent: " + getPacketsSent() + "\n"); >>> > + sb.append("Connections: " + getNumAliveClientConnections() + >>> "\n"); >>> > + >>> > if (provider != null) { >>> > sb.append("Outstanding: " + getOutstandingRequests() + >>> "\n"); >>> > sb.append("Zxid: 0x"+ >>> Long.toHexString(getLastProcessedZxid())+ "\n"); >>> > @@ -123,7 +132,6 @@ public class ServerStats { >>> > packetsReceived = 0; >>> > packetsSent = 0; >>> > } >>> > - >>> > synchronized public void reset() { >>> > resetLatency(); >>> > resetRequestCounters(); >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -109,7 +109,7 @@ public class ZooKeeperServer implements >>> > private ServerCnxnFactory serverCnxnFactory; >>> > >>> > private final ServerStats serverStats; >>> > - >>> > + >>> > void removeCnxn(ServerCnxn cnxn) { >>> > zkDb.removeCnxn(cnxn); >>> > } >>> > @@ -254,7 +254,6 @@ public class ZooKeeperServer implements >>> > } >>> > } >>> > >>> > - >>> > /** >>> > * This should be called from a synchronized block on this! >>> > */ >>> > @@ -678,6 +677,14 @@ public class ZooKeeperServer implements >>> > } >>> > >>> > /** >>> > + * return the total number of client connections that are alive >>> > + * to this server >>> > + */ >>> > + public int getNumAliveConnections() { >>> > + return serverCnxnFactory.getNumAliveConnections(); >>> > + } >>> > + >>> > + /** >>> > * trunccate the log to get in sync with others >>> > * if in a quorum >>> > * @param zxid the zxid that it needs to get in sync >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -140,4 +140,8 @@ public class ZooKeeperServerBean impleme >>> > serverStats.resetRequestCounters(); >>> > serverStats.resetLatency(); >>> > } >>> > + >>> > + public long getNumAliveConnections() { >>> > + return zks.getNumAliveConnections(); >>> > + } >>> > } >>> > >>> > Modified: >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMXBean.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMXBean.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMXBean.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMXBean.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -103,4 +103,8 @@ public interface ZooKeeperServerMXBean { >>> > * Reset max latency statistics only. >>> > */ >>> > public void resetMaxLatency(); >>> > + /** >>> > + * @return number of alive client connections >>> > + */ >>> > + public long getNumAliveConnections(); >>> > } >>> > >>> > Modified: >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -212,6 +212,10 @@ public class Zab1_0Test { >>> > } >>> > public void closeAll() { >>> > } >>> > + @Override >>> > + public int getNumAliveConnections() { >>> > + return 0; >>> > + } >>> > } >>> > static Socket[] getSocketPair() throws IOException { >>> > ServerSocket ss = new ServerSocket(); >>> > >>> > Modified: >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java >>> > URL: >>> http://svn.apache.org/viewvc/zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java?rev=1225200&r1=1225199&r2=1225200&view=diff >>> > >>> ============================================================================== >>> > --- >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java >>> (original) >>> > +++ >>> zookeeper/trunk/src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java >>> Wed Dec 28 14:55:37 2011 >>> > @@ -94,6 +94,9 @@ public class FourLetterWordsTest extends >>> > verify("srvr", "Outstanding"); >>> > verify("cons", "queued"); >>> > verify("mntr", "zk_server_state\tstandalone"); >>> > + verify("mntr", "num_alive_connections"); >>> > + verify("stat", "Connections"); >>> > + verify("srvr", "Connections"); >>> > } >>> > >>> > private String sendRequest(String cmd) throws IOException { >>> > @@ -136,6 +139,8 @@ public class FourLetterWordsTest extends >>> > line = in.readLine(); >>> > Assert.assertTrue(Pattern.matches("^Sent: \\d+$", line)); >>> > line = in.readLine(); >>> > + Assert.assertTrue(Pattern.matches("^Connections: \\d+$", >>> line)); >>> > + line = in.readLine(); >>> > Assert.assertTrue(Pattern.matches("^Outstanding: \\d+$", line)); >>> > line = in.readLine(); >>> > Assert.assertTrue(Pattern.matches("^Zxid: 0x[\\da-fA-F]+$", >>> line)); >>> > >>> > >>> >>
