Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.4.8
    • Fix Version/s: 3.4.10, 3.5.3, 3.6.0
    • Component/s: jmx, server
    • Labels:
      None

      Description

      In attempting to upgrade Solr's ZooKeeper dependency from 3.4.6 to 3.4.8 (SOLR-8724) I ran into test failures where attempts to create a node in a newly started standalone ZooKeeperServer were failing because of an assertion in MBeanRegistry.

      ZooKeeperServer.startup() first sets up its request processor chain then registers itself in JMX, but if a connection comes in before the server's JMX registration happens, registration of the connection will fail because it trips the assertion that (effectively) its parent (the server) has already registered itself.

      ZooKeeperServer.java
          public synchronized void startup() {
              if (sessionTracker == null) {
                  createSessionTracker();
              }
              startSessionTracker();
              setupRequestProcessors();
      
              registerJMX();
      
              state = State.RUNNING;
              notifyAll();
          }
      
      MBeanRegistry.java
          public void register(ZKMBeanInfo bean, ZKMBeanInfo parent)
              throws JMException
          {
              assert bean != null;
              String path = null;
              if (parent != null) {
                  path = mapBean2Path.get(parent);
                  assert path != null;
              }
      

      This problem appears to be new with ZK 3.4.8 - AFAIK Solr never had this issue with ZK 3.4.6.

      1. release-3.4.8-extra-logging.patch
        3 kB
        Steve Rowe
      2. TestZkStandaloneJMXRegistrationRaceConcurrent.java
        2 kB
        Steve Rowe
      3. zk-3.4.8-MBeanRegistry.log
        10 kB
        Steve Rowe
      4. zk-3.4.8-NPE.log
        12 kB
        Steve Rowe
      5. ZOOKEEPER-2383.patch
        22 kB
        Rakesh R
      6. ZOOKEEPER-2383.patch
        15 kB
        Rakesh R
      7. ZOOKEEPER-2383.patch
        14 kB
        Rakesh R
      8. ZOOKEEPER-2383.patch
        14 kB
        Rakesh R
      9. ZOOKEEPER-2383-br-3-4.patch
        14 kB
        Rakesh R

        Issue Links

          Activity

          Hide
          steve_rowe Steve Rowe added a comment -

          This program triggers the problem for me roughly 10% of the time with ZK 3.4.8 - note that if I don't use a thread to start ZooKeeperServer, the connection always comes in after the server has had a chance to register itself with JMX (imports omitted - attaching full file here in a sec):

          TestZkStandaloneJMXRegistrationRaceConcurrent.java
          public class TestZkStandaloneJMXRegistrationRaceConcurrent {
            public static void main(String[] args) throws IOException, InterruptedException, KeeperException {
              class ServerThread extends Thread {
                private ZooKeeperServer server;
                private ServerCnxnFactory cnxnFactory;
                @Override public void run() {
                  try {
                    File tempDir = Files.createTempDirectory(FileSystems.getDefault().getPath("."),"test").toFile();
                    FileTxnSnapLog txnSnapLog = new FileTxnSnapLog(tempDir, tempDir);
                    server = new ZooKeeperServer
                      (txnSnapLog, 2000, 2000, 4000, null, new ZKDatabase(txnSnapLog));
                    cnxnFactory = ServerCnxnFactory.createFactory(55555, -1);
                    cnxnFactory.startup(server);
                  } catch (IOException e) {
                    throw new RuntimeException(e);
                  } catch (InterruptedException e) { 
                    Thread.currentThread().interrupt();
                  }
                }
                public void shutdown() throws IOException, InterruptedException {
                  cnxnFactory.shutdown();
                  cnxnFactory.join();
                  server.shutdown();
                }
              }
              ServerThread serverThread = new ServerThread();
              serverThread.setDaemon(true);
              serverThread.start();
              Thread.sleep(3);
              ZooKeeper zk = new ZooKeeper("127.0.0.1:55555", 45000, new Watcher() {
                  public void process(WatchedEvent event) {} });
              zk.create("/testing123", new byte[]{}, Ids.OPEN_ACL_UNSAFE, CreateMode.EPHEMERAL);
              serverThread.shutdown();
              serverThread.join();
            }
          }
          

          Here's an excerpt from a log exhibiting the failure - I'll also attach the full log (I've added some logging to ZK 3.4.8 - I'll attach a patch showing those additions here in a minute):

          2016-03-08 11:32:08,414 [myid:] - WARN  [SyncThread:0:MBeanRegistry@100] - bean 'Connections/127.0.0.1/0x153571244a70000' with parent 'StandaloneServer_port55555' has null path.
          java.lang.Throwable: 
          	at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:98)
          	at org.apache.zookeeper.server.ServerCnxnFactory.registerConnection(ServerCnxnFactory.java:147)
          	at org.apache.zookeeper.server.ZooKeeperServer.finishSessionInit(ZooKeeperServer.java:613)
          	at org.apache.zookeeper.server.FinalRequestProcessor.processRequest(FinalRequestProcessor.java:181)
          	at org.apache.zookeeper.server.SyncRequestProcessor.flush(SyncRequestProcessor.java:200)
          	at org.apache.zookeeper.server.SyncRequestProcessor.run(SyncRequestProcessor.java:131)
          2016-03-08 11:32:08,414 [myid:] - WARN  [Thread-0:MBeanRegistry@118] - registered bean 'StandaloneServer_port55555' with parent 'null' at path '/'
          java.lang.Throwable: 
          	at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:116)
          	at org.apache.zookeeper.server.ZooKeeperServer.registerJMX(ZooKeeperServer.java:385)
          	at org.apache.zookeeper.server.ZooKeeperServer.startup(ZooKeeperServer.java:418)
          	at org.apache.zookeeper.server.NIOServerCnxnFactory.startup(NIOServerCnxnFactory.java:119)
          	at TestZkStandaloneJMXRegistrationRaceConcurrent$1ServerThread.run(TestZkStandaloneJMXRegistrationRaceConcurrent.java:29)
          2016-03-08 11:32:08,415 [myid:] - ERROR [SyncThread:0:ZooKeeperCriticalThread@49] - Severe unrecoverable error, from thread : SyncThread:0
          java.lang.AssertionError
          	at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:104)
          	at org.apache.zookeeper.server.ServerCnxnFactory.registerConnection(ServerCnxnFactory.java:147)
          	at org.apache.zookeeper.server.ZooKeeperServer.finishSessionInit(ZooKeeperServer.java:613)
          	at org.apache.zookeeper.server.FinalRequestProcessor.processRequest(FinalRequestProcessor.java:181)
          	at org.apache.zookeeper.server.SyncRequestProcessor.flush(SyncRequestProcessor.java:200)
          	at org.apache.zookeeper.server.SyncRequestProcessor.run(SyncRequestProcessor.java:131)
          2016-03-08 11:32:08,416 [myid:] - WARN  [Thread-0:MBeanRegistry@118] - registered bean 'InMemoryDataTree' with parent 'StandaloneServer_port55555' at path '/StandaloneServer_port55555'
          java.lang.Throwable: 
          	at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:116)
          	at org.apache.zookeeper.server.ZooKeeperServer.registerJMX(ZooKeeperServer.java:389)
          	at org.apache.zookeeper.server.ZooKeeperServer.startup(ZooKeeperServer.java:418)
          	at org.apache.zookeeper.server.NIOServerCnxnFactory.startup(NIOServerCnxnFactory.java:119)
          	at TestZkStandaloneJMXRegistrationRaceConcurrent$1ServerThread.run(TestZkStandaloneJMXRegistrationRaceConcurrent.java:29)
          
          Show
          steve_rowe Steve Rowe added a comment - This program triggers the problem for me roughly 10% of the time with ZK 3.4.8 - note that if I don't use a thread to start ZooKeeperServer, the connection always comes in after the server has had a chance to register itself with JMX (imports omitted - attaching full file here in a sec): TestZkStandaloneJMXRegistrationRaceConcurrent.java public class TestZkStandaloneJMXRegistrationRaceConcurrent { public static void main( String [] args) throws IOException, InterruptedException, KeeperException { class ServerThread extends Thread { private ZooKeeperServer server; private ServerCnxnFactory cnxnFactory; @Override public void run() { try { File tempDir = Files.createTempDirectory(FileSystems.getDefault().getPath( "." ), "test" ).toFile(); FileTxnSnapLog txnSnapLog = new FileTxnSnapLog(tempDir, tempDir); server = new ZooKeeperServer (txnSnapLog, 2000, 2000, 4000, null , new ZKDatabase(txnSnapLog)); cnxnFactory = ServerCnxnFactory.createFactory(55555, -1); cnxnFactory.startup(server); } catch (IOException e) { throw new RuntimeException(e); } catch (InterruptedException e) { Thread .currentThread().interrupt(); } } public void shutdown() throws IOException, InterruptedException { cnxnFactory.shutdown(); cnxnFactory.join(); server.shutdown(); } } ServerThread serverThread = new ServerThread(); serverThread.setDaemon( true ); serverThread.start(); Thread .sleep(3); ZooKeeper zk = new ZooKeeper( "127.0.0.1:55555" , 45000, new Watcher() { public void process(WatchedEvent event) {} }); zk.create( "/testing123" , new byte []{}, Ids.OPEN_ACL_UNSAFE, CreateMode.EPHEMERAL); serverThread.shutdown(); serverThread.join(); } } Here's an excerpt from a log exhibiting the failure - I'll also attach the full log (I've added some logging to ZK 3.4.8 - I'll attach a patch showing those additions here in a minute): 2016-03-08 11:32:08,414 [myid:] - WARN [SyncThread:0:MBeanRegistry@100] - bean 'Connections/127.0.0.1/0x153571244a70000' with parent 'StandaloneServer_port55555' has null path. java.lang.Throwable: at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:98) at org.apache.zookeeper.server.ServerCnxnFactory.registerConnection(ServerCnxnFactory.java:147) at org.apache.zookeeper.server.ZooKeeperServer.finishSessionInit(ZooKeeperServer.java:613) at org.apache.zookeeper.server.FinalRequestProcessor.processRequest(FinalRequestProcessor.java:181) at org.apache.zookeeper.server.SyncRequestProcessor.flush(SyncRequestProcessor.java:200) at org.apache.zookeeper.server.SyncRequestProcessor.run(SyncRequestProcessor.java:131) 2016-03-08 11:32:08,414 [myid:] - WARN [Thread-0:MBeanRegistry@118] - registered bean 'StandaloneServer_port55555' with parent 'null' at path '/' java.lang.Throwable: at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:116) at org.apache.zookeeper.server.ZooKeeperServer.registerJMX(ZooKeeperServer.java:385) at org.apache.zookeeper.server.ZooKeeperServer.startup(ZooKeeperServer.java:418) at org.apache.zookeeper.server.NIOServerCnxnFactory.startup(NIOServerCnxnFactory.java:119) at TestZkStandaloneJMXRegistrationRaceConcurrent$1ServerThread.run(TestZkStandaloneJMXRegistrationRaceConcurrent.java:29) 2016-03-08 11:32:08,415 [myid:] - ERROR [SyncThread:0:ZooKeeperCriticalThread@49] - Severe unrecoverable error, from thread : SyncThread:0 java.lang.AssertionError at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:104) at org.apache.zookeeper.server.ServerCnxnFactory.registerConnection(ServerCnxnFactory.java:147) at org.apache.zookeeper.server.ZooKeeperServer.finishSessionInit(ZooKeeperServer.java:613) at org.apache.zookeeper.server.FinalRequestProcessor.processRequest(FinalRequestProcessor.java:181) at org.apache.zookeeper.server.SyncRequestProcessor.flush(SyncRequestProcessor.java:200) at org.apache.zookeeper.server.SyncRequestProcessor.run(SyncRequestProcessor.java:131) 2016-03-08 11:32:08,416 [myid:] - WARN [Thread-0:MBeanRegistry@118] - registered bean 'InMemoryDataTree' with parent 'StandaloneServer_port55555' at path '/StandaloneServer_port55555' java.lang.Throwable: at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:116) at org.apache.zookeeper.server.ZooKeeperServer.registerJMX(ZooKeeperServer.java:389) at org.apache.zookeeper.server.ZooKeeperServer.startup(ZooKeeperServer.java:418) at org.apache.zookeeper.server.NIOServerCnxnFactory.startup(NIOServerCnxnFactory.java:119) at TestZkStandaloneJMXRegistrationRaceConcurrent$1ServerThread.run(TestZkStandaloneJMXRegistrationRaceConcurrent.java:29)
          Hide
          steve_rowe Steve Rowe added a comment -

          Attaching files:

          Test case: TestZkStandaloneJMXRegistrationRaceConcurrent.java
          ZK 3.4.8 source patch to add JXM logging: release-3.4.8-extra-logging.patch
          Log file exhibiting failure: zk-3.4.8-MBeanRegistry.log

          Show
          steve_rowe Steve Rowe added a comment - Attaching files: Test case: TestZkStandaloneJMXRegistrationRaceConcurrent.java ZK 3.4.8 source patch to add JXM logging: release-3.4.8-extra-logging.patch Log file exhibiting failure: zk-3.4.8-MBeanRegistry.log
          Hide
          steve_rowe Steve Rowe added a comment -

          Sometimes the attached test case will trigger a NullPointerException in ZooKeeperServer.createSession() - see attached zk-3.4.8-NPE.log. For some reason I never saw this failure mode in Solr tests using ZK 3.4.8.

          2016-03-08 11:29:00,374 [myid:] - WARN  [NIOServerCxn.Factory:0.0.0.0/0.0.0.0:55555:NIOServerCnxnFactory@213] - Ignoring unexpected runtime e
          xception
          java.lang.NullPointerException
                  at org.apache.zookeeper.server.ZooKeeperServer.createSession(ZooKeeperServer.java:569)
                  at org.apache.zookeeper.server.ZooKeeperServer.processConnectRequest(ZooKeeperServer.java:902)
                  at org.apache.zookeeper.server.NIOServerCnxn.readConnectRequest(NIOServerCnxn.java:418)
                  at org.apache.zookeeper.server.NIOServerCnxn.readPayload(NIOServerCnxn.java:198)
                  at org.apache.zookeeper.server.NIOServerCnxn.doIO(NIOServerCnxn.java:244)
                  at org.apache.zookeeper.server.NIOServerCnxnFactory.run(NIOServerCnxnFactory.java:203)
                  at java.lang.Thread.run(Thread.java:745)
          
          Show
          steve_rowe Steve Rowe added a comment - Sometimes the attached test case will trigger a NullPointerException in ZooKeeperServer.createSession() - see attached zk-3.4.8-NPE.log. For some reason I never saw this failure mode in Solr tests using ZK 3.4.8. 2016-03-08 11:29:00,374 [myid:] - WARN [NIOServerCxn.Factory:0.0.0.0/0.0.0.0:55555:NIOServerCnxnFactory@213] - Ignoring unexpected runtime e xception java.lang.NullPointerException at org.apache.zookeeper.server.ZooKeeperServer.createSession(ZooKeeperServer.java:569) at org.apache.zookeeper.server.ZooKeeperServer.processConnectRequest(ZooKeeperServer.java:902) at org.apache.zookeeper.server.NIOServerCnxn.readConnectRequest(NIOServerCnxn.java:418) at org.apache.zookeeper.server.NIOServerCnxn.readPayload(NIOServerCnxn.java:198) at org.apache.zookeeper.server.NIOServerCnxn.doIO(NIOServerCnxn.java:244) at org.apache.zookeeper.server.NIOServerCnxnFactory.run(NIOServerCnxnFactory.java:203) at java.lang.Thread.run(Thread.java:745)
          Hide
          fpj Flavio Junqueira added a comment - - edited

          Steve Rowe Thanks for reporting this issue. According to git blame, the latest changes around the startup method in ZooKeeperServer are due to ZOOKEEPER-1907, which actually turned out to be quite problematic, so this could be another issue due to that patch, I'm not sure.

          91f579e4 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Hongchao Deng          2015-08-17 20:52:07 +0000  411)     public synchronized void startup() {
          55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Mahadev Konar          2012-01-31 06:50:06 +0000  412)         if (sessionTracker == null) {
          55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Mahadev Konar          2012-01-31 06:50:06 +0000  413)             createSessionTracker();
          55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Mahadev Konar          2012-01-31 06:50:06 +0000  414)         }
          55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Mahadev Konar          2012-01-31 06:50:06 +0000  415)         startSessionTracker();
          097b7979 zookeeper/java/src/com/yahoo/zookeeper/server/ZooKeeperServer.java (Benjamin Reed          2008-05-12 23:01:25 +0000  416)         setupRequestProcessors();
          87e1e030 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Patrick D. Hunt        2009-01-15 22:57:14 +0000  417) 
          87e1e030 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Patrick D. Hunt        2009-01-15 22:57:14 +0000  418)         registerJMX();
          87e1e030 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Patrick D. Hunt        2009-01-15 22:57:14 +0000  419) 
          91f579e4 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Hongchao Deng          2015-08-17 20:52:07 +0000  420)         state = State.RUNNING;
          91f579e4 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java     (Hongchao Deng          2015-08-17 20:52:07 +0000  421)         notifyAll();
          097b7979 zookeeper/java/src/com/yahoo/zookeeper/server/ZooKeeperServer.java (Benjamin Reed          2008-05-12 23:01:25 +0000  422)     }
          
          
          commit 91f579e40755de870ed9123c8fd55925517d9aa6
          Author: Hongchao Deng <hdeng@apache.org>
          Date:   Mon Aug 17 20:52:07 2015 +0000
          
              ZOOKEEPER-1907 Improve Thread handling (Rakesh R via hdeng)
              
              git-svn-id: https://svn.apache.org/repos/asf/zookeeper/branches/branch-3.4@1696337 13f79535-47bb-0310-9956-ffa450edef68
          

          Rakesh R could you have a look, please?

          CC Raul Gutierrez Segales Patrick Hunt

          Show
          fpj Flavio Junqueira added a comment - - edited Steve Rowe Thanks for reporting this issue. According to git blame, the latest changes around the startup method in ZooKeeperServer are due to ZOOKEEPER-1907 , which actually turned out to be quite problematic, so this could be another issue due to that patch, I'm not sure. 91f579e4 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Hongchao Deng 2015-08-17 20:52:07 +0000 411) public synchronized void startup() { 55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Mahadev Konar 2012-01-31 06:50:06 +0000 412) if (sessionTracker == null) { 55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Mahadev Konar 2012-01-31 06:50:06 +0000 413) createSessionTracker(); 55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Mahadev Konar 2012-01-31 06:50:06 +0000 414) } 55b03fce src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Mahadev Konar 2012-01-31 06:50:06 +0000 415) startSessionTracker(); 097b7979 zookeeper/java/src/com/yahoo/zookeeper/server/ZooKeeperServer.java (Benjamin Reed 2008-05-12 23:01:25 +0000 416) setupRequestProcessors(); 87e1e030 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Patrick D. Hunt 2009-01-15 22:57:14 +0000 417) 87e1e030 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Patrick D. Hunt 2009-01-15 22:57:14 +0000 418) registerJMX(); 87e1e030 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Patrick D. Hunt 2009-01-15 22:57:14 +0000 419) 91f579e4 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Hongchao Deng 2015-08-17 20:52:07 +0000 420) state = State.RUNNING; 91f579e4 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (Hongchao Deng 2015-08-17 20:52:07 +0000 421) notifyAll(); 097b7979 zookeeper/java/src/com/yahoo/zookeeper/server/ZooKeeperServer.java (Benjamin Reed 2008-05-12 23:01:25 +0000 422) } commit 91f579e40755de870ed9123c8fd55925517d9aa6 Author: Hongchao Deng <hdeng@apache.org> Date: Mon Aug 17 20:52:07 2015 +0000 ZOOKEEPER-1907 Improve Thread handling (Rakesh R via hdeng) git-svn-id: https://svn.apache.org/repos/asf/zookeeper/branches/branch-3.4@1696337 13f79535-47bb-0310-9956-ffa450edef68 Rakesh R could you have a look, please? CC Raul Gutierrez Segales Patrick Hunt
          Hide
          steve_rowe Steve Rowe added a comment -

          FWIW, I was able to work around the problem in Solr tests by subclassing ZooKeeperServer and ordering server JMX registration before setting up its request processing pipeline, like so:

           private class TestZooKeeperServer extends ZooKeeperServer {
              @Override protected void registerJMX() {
                // no-op - super.registerJMX() is called in overridden startup()
              }
              /** Register in JMX before starting the request processors. */
              @Override public void startup() {
                super.registerJMX();
                super.startup();
              }
              public TestZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime,
                                         int minSessionTimeout, int maxSessionTimeout,
                                         DataTreeBuilder treeBuilder, ZKDatabase zkDb) {
                super(txnLogFactory, tickTime, minSessionTimeout, maxSessionTimeout, treeBuilder, zkDb);
              }
            }
          
          Show
          steve_rowe Steve Rowe added a comment - FWIW, I was able to work around the problem in Solr tests by subclassing ZooKeeperServer and ordering server JMX registration before setting up its request processing pipeline, like so: private class TestZooKeeperServer extends ZooKeeperServer { @Override protected void registerJMX() { // no-op - super .registerJMX() is called in overridden startup() } /** Register in JMX before starting the request processors. */ @Override public void startup() { super .registerJMX(); super .startup(); } public TestZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime, int minSessionTimeout, int maxSessionTimeout, DataTreeBuilder treeBuilder, ZKDatabase zkDb) { super (txnLogFactory, tickTime, minSessionTimeout, maxSessionTimeout, treeBuilder, zkDb); } }
          Hide
          steve_rowe Steve Rowe added a comment -

          Similarly to the subclassed ZooKeeperServer above, if I apply the following patch to ZK 3.4.8 and run the attached test case with it, the JMX registration race no longer happens:

          Index: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
          ===================================================================
          --- src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java	(revision 1732157)
          +++ src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java	(working copy)
          @@ -413,10 +413,9 @@
                       createSessionTracker();
                   }
                   startSessionTracker();
          +        registerJMX();
                   setupRequestProcessors();
           
          -        registerJMX();
          -
                   state = State.RUNNING;
                   notifyAll();
               }
          
          Show
          steve_rowe Steve Rowe added a comment - Similarly to the subclassed ZooKeeperServer above, if I apply the following patch to ZK 3.4.8 and run the attached test case with it, the JMX registration race no longer happens: Index: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java =================================================================== --- src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (revision 1732157) +++ src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (working copy) @@ -413,10 +413,9 @@ createSessionTracker(); } startSessionTracker(); + registerJMX(); setupRequestProcessors(); - registerJMX(); - state = State.RUNNING; notifyAll(); }
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Steve Rowe for reporting this issue and good analysis.

          According to git blame, the latest changes around the startup method in ZooKeeperServer are due to ZOOKEEPER-1907, which actually turned out to be quite problematic, so this could be another issue due to that patch, I'm not sure.

          Flavio Junqueira, sure I'm happy to investigate this. To understand the impact of ZOOKEEPER-1907, first I took the code before ZOOKEEPER-1907 commit version da3e7e0d4b66ac5a25d40ae2d0102b1b57994b62. I've debugged the code and able to re-produce the issue even without ZOOKEEPER-1907 changes.

          Coming back to the issues reported in this jira, there are two issues. IIUC, both the cases are due to the race between server startup and processing a client connection request. I've tried an attempt to figure it out, please see the below sequence that creating the trouble.

          1. NullPointerException while creating session
            2016-03-08 11:29:00,374 [myid:] - WARN  [NIOServerCxn.Factory:0.0.0.0/0.0.0.0:55555:NIOServerCnxnFactory@213] - Ignoring unexpected runtime exception
            java.lang.NullPointerException
            	at org.apache.zookeeper.server.ZooKeeperServer.createSession(ZooKeeperServer.java:569)
            

            Thread-1: Starting the server
            1=> Invoked cnxnFactory.startup(server);
            2=> Started NIOServerCxn.Factory thread and register OP_ACCEPT to accept connections
            3=> sets zookeeper server to the connection factory
            4=> loads zookeeper data
            5=> Assume server is about to invoke zks.startup(); and sessionTracker is not yet initialized.
            Thread-2: creating client connection
            1=> sends connection request to the server
            2=> NIOServerCnxn reads the request and invokes NIOServerCnxn#readConnectRequest()
            3=> It then calls zkServer.processConnectRequest(this, incomingBuffer);
            4=> While processing the request it needs sessionTracker reference, but this is not yet initialized and the server is still in the startup phase causing the NPE error.

          2. MBeanRegistry throws assertion error due to parent doesn't exists
            2016-03-08 11:29:00,449 [myid:] - WARN  [Thread-0:MBeanRegistry@118] - registered bean 'InMemoryDataTree' with parent 'StandaloneServer_port55555' at path '/StandaloneServer_port55555'
            java.lang.Throwable: 
            	at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:116)
            

            Thread-1: Starting the server
            1=> Invoked cnxnFactory.startup(server);
            2=> Started NIOServerCxn.Factory thread and register OP_ACCEPT to accept connections
            3=> sets zookeeper server to the connection factory
            4=> loads zookeeper data
            5=> Server invoked zks.startup();
            6=> Started session tracker
            7=> Finished settingup RequestProcessors
            8=> Invoked ZooKeeperServer#registerJMX();
            9=> Now assume ZooKeeperServer has initialized jmxServerBean = new ZooKeeperServerBean(this); and about to register the bean in the registry MBeanRegistry.getInstance().register(jmxServerBean, null);
            Thread-2: creating client connection
            1=> sends connection request to the server
            2=> NIOServerCnxn reads the request and invokes NIOServerCnxn#readConnectRequest()
            3=> It then calls zkServer.processConnectRequest(this, incomingBuffer);
            4=> Since all the request processors are ready, it successfully creates the session and goes to register the connection bean
            5=> Now, it will invoke zkServer.finishSessionInit(). Here it invokes serverCnxnFactory.registerConnection(cnxn); and hitting the path error.

          Show
          rakeshr Rakesh R added a comment - Thanks Steve Rowe for reporting this issue and good analysis. According to git blame, the latest changes around the startup method in ZooKeeperServer are due to ZOOKEEPER-1907 , which actually turned out to be quite problematic, so this could be another issue due to that patch, I'm not sure. Flavio Junqueira , sure I'm happy to investigate this. To understand the impact of ZOOKEEPER-1907 , first I took the code before ZOOKEEPER-1907 commit version da3e7e0d4b66ac5a25d40ae2d0102b1b57994b62 . I've debugged the code and able to re-produce the issue even without ZOOKEEPER-1907 changes. Coming back to the issues reported in this jira, there are two issues. IIUC, both the cases are due to the race between server startup and processing a client connection request. I've tried an attempt to figure it out, please see the below sequence that creating the trouble. NullPointerException while creating session 2016-03-08 11:29:00,374 [myid:] - WARN [NIOServerCxn.Factory:0.0.0.0/0.0.0.0:55555:NIOServerCnxnFactory@213] - Ignoring unexpected runtime exception java.lang.NullPointerException at org.apache.zookeeper.server.ZooKeeperServer.createSession(ZooKeeperServer.java:569) Thread-1: Starting the server 1=> Invoked cnxnFactory.startup(server); 2=> Started NIOServerCxn.Factory thread and register OP_ACCEPT to accept connections 3=> sets zookeeper server to the connection factory 4=> loads zookeeper data 5=> Assume server is about to invoke zks.startup(); and sessionTracker is not yet initialized. Thread-2: creating client connection 1=> sends connection request to the server 2=> NIOServerCnxn reads the request and invokes NIOServerCnxn#readConnectRequest() 3=> It then calls zkServer.processConnectRequest(this, incomingBuffer); 4=> While processing the request it needs sessionTracker reference, but this is not yet initialized and the server is still in the startup phase causing the NPE error. MBeanRegistry throws assertion error due to parent doesn't exists 2016-03-08 11:29:00,449 [myid:] - WARN [ Thread -0:MBeanRegistry@118] - registered bean 'InMemoryDataTree' with parent 'StandaloneServer_port55555' at path '/StandaloneServer_port55555' java.lang.Throwable: at org.apache.zookeeper.jmx.MBeanRegistry.register(MBeanRegistry.java:116) Thread-1: Starting the server 1=> Invoked cnxnFactory.startup(server); 2=> Started NIOServerCxn.Factory thread and register OP_ACCEPT to accept connections 3=> sets zookeeper server to the connection factory 4=> loads zookeeper data 5=> Server invoked zks.startup(); 6=> Started session tracker 7=> Finished settingup RequestProcessors 8=> Invoked ZooKeeperServer#registerJMX(); 9=> Now assume ZooKeeperServer has initialized jmxServerBean = new ZooKeeperServerBean(this); and about to register the bean in the registry MBeanRegistry.getInstance().register(jmxServerBean, null); Thread-2: creating client connection 1=> sends connection request to the server 2=> NIOServerCnxn reads the request and invokes NIOServerCnxn#readConnectRequest() 3=> It then calls zkServer.processConnectRequest(this, incomingBuffer); 4=> Since all the request processors are ready, it successfully creates the session and goes to register the connection bean 5=> Now, it will invoke zkServer.finishSessionInit() . Here it invokes serverCnxnFactory.registerConnection(cnxn); and hitting the path error.
          Hide
          rakeshr Rakesh R added a comment -

          I think I found the code changes which results in this bug. Its due to the change in startup sequence - ZOOKEEPER-2026.

          Here it just moved up setting 'zkServer' reference before starting up the standalone server fully. Internally, this 'zkServer' reference is used to see ZooKeeperServer's running status. In the defect scenario, the standalone server is partially started(by Thread1) and simultaneously client(by Thread2) send a connection request. Since the 'zkServer' is not null it proceeds to process the connection request and causing the trouble. Please refer my previous comment to understand the problematic call sequence.

          ZooKeeperServer.java
          
              private void readConnectRequest() throws IOException, InterruptedException {
                  if (zkServer == null) {
                      throw new IOException("ZooKeeperServer not running");
                  }
                  zkServer.processConnectRequest(this, incomingBuffer);
                  initialized = true;
              }
          

          Probably should use server RUNNING state instead of "zkServer == null" checks to know the running status. Server is updating the state to RUNNING after starting all the services.

          ZooKeeperServer.java
          
              public synchronized void startup() {
                  if (sessionTracker == null) {
                      createSessionTracker();
                  }
                  startSessionTracker();
                  setupRequestProcessors();
          
                  registerJMX();
          
                  state = State.RUNNING;
                  notifyAll();
              }
          
          Show
          rakeshr Rakesh R added a comment - I think I found the code changes which results in this bug. Its due to the change in startup sequence - ZOOKEEPER-2026 . Here it just moved up setting 'zkServer' reference before starting up the standalone server fully. Internally, this 'zkServer' reference is used to see ZooKeeperServer's running status. In the defect scenario, the standalone server is partially started(by Thread1) and simultaneously client(by Thread2) send a connection request. Since the 'zkServer' is not null it proceeds to process the connection request and causing the trouble. Please refer my previous comment to understand the problematic call sequence. ZooKeeperServer.java private void readConnectRequest() throws IOException, InterruptedException { if (zkServer == null ) { throw new IOException( "ZooKeeperServer not running" ); } zkServer.processConnectRequest( this , incomingBuffer); initialized = true ; } Probably should use server RUNNING state instead of "zkServer == null" checks to know the running status. Server is updating the state to RUNNING after starting all the services. ZooKeeperServer.java public synchronized void startup() { if (sessionTracker == null ) { createSessionTracker(); } startSessionTracker(); setupRequestProcessors(); registerJMX(); state = State.RUNNING; notifyAll(); }
          Hide
          jbrosenberg@gmail.com Jason Rosenberg added a comment -

          I've run into similar issues, with the register call causing an AssertionError, which is not caught anywhere, which results in test failures (especially if you have multiple QuorumPeers running in the same jvm during tests). JMX registration gets confused if you have individual nodes starting and stopping. (Asserts are only enabled for us during testing/ci, so it doesn't affect production).
          I worked around it be calling

          MBeanRegistry.setInstance(new FakeMBeanRegistry())

          . Where

          FakeMBeanRegistry

          looks like:

            public static class FakeMBeanRegistry extends MBeanRegistry {
              @Override public void register(ZKMBeanInfo bean, ZKMBeanInfo parent) throws JMException {}
              @Override public void unregister(ZKMBeanInfo bean) {}
              @Override public void unregisterAll() {}
            }
          

          Maybe there should be a flag to disable all JMX registrations (e.g. for most tests), and it could be handled with something like this fake class.

          Show
          jbrosenberg@gmail.com Jason Rosenberg added a comment - I've run into similar issues, with the register call causing an AssertionError, which is not caught anywhere, which results in test failures (especially if you have multiple QuorumPeers running in the same jvm during tests). JMX registration gets confused if you have individual nodes starting and stopping. (Asserts are only enabled for us during testing/ci, so it doesn't affect production). I worked around it be calling MBeanRegistry.setInstance( new FakeMBeanRegistry()) . Where FakeMBeanRegistry looks like: public static class FakeMBeanRegistry extends MBeanRegistry { @Override public void register(ZKMBeanInfo bean, ZKMBeanInfo parent) throws JMException {} @Override public void unregister(ZKMBeanInfo bean) {} @Override public void unregisterAll() {} } Maybe there should be a flag to disable all JMX registrations (e.g. for most tests), and it could be handled with something like this fake class.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Jason Rosenberg. Disbaling or mocking may affect the org.apache.zookeeper.testJMXEnv#ensureAll(expectednames) verification, isn't it?. In unit tests, it verifies all the registered jmx beans to ensure that the server is started fully.

          Flavio Junqueira, I have one idea to fix this issue by modifying the condition zkServer == null with zkServer.isRunning() status check. After seeing the code changes, I feel to push this logic carefully after 3.5.2 release, which is waiting to be released soon. Also, I think needs to identify and add more unit test cases covering server startup/shutdown/restart corner cases in order to push this change. Now, I'm planning to revert ZOOKEEPER-2026 committed changes and re-open the jira. Does this makes sense to you?

          Show
          rakeshr Rakesh R added a comment - Thanks Jason Rosenberg . Disbaling or mocking may affect the org.apache.zookeeper.testJMXEnv#ensureAll(expectednames) verification, isn't it?. In unit tests, it verifies all the registered jmx beans to ensure that the server is started fully. Flavio Junqueira , I have one idea to fix this issue by modifying the condition zkServer == null with zkServer.isRunning() status check. After seeing the code changes, I feel to push this logic carefully after 3.5.2 release, which is waiting to be released soon. Also, I think needs to identify and add more unit test cases covering server startup/shutdown/restart corner cases in order to push this change. Now, I'm planning to revert ZOOKEEPER-2026 committed changes and re-open the jira. Does this makes sense to you?
          Hide
          jbrosenberg@gmail.com Jason Rosenberg added a comment -

          Yes, agreed, you wouldn't use a FakeMBeanRegistry for tests that are actually testing the jmx functionality!
          However, I think it makes sense to have it be something easily configurable/settable for most tests (and especially for tests which launch multiple peers in the same jvm).

          Show
          jbrosenberg@gmail.com Jason Rosenberg added a comment - Yes, agreed, you wouldn't use a FakeMBeanRegistry for tests that are actually testing the jmx functionality! However, I think it makes sense to have it be something easily configurable/settable for most tests (and especially for tests which launch multiple peers in the same jvm).
          Hide
          ozawa Tsuyoshi Ozawa added a comment -

          Rakesh R hi Rakesh, how is the progress of this issue?

          Show
          ozawa Tsuyoshi Ozawa added a comment - Rakesh R hi Rakesh, how is the progress of this issue?
          Hide
          rakeshr Rakesh R added a comment -

          Tsuyoshi Ozawa, thanks for the interest. I was busy with other things and will give priority to this issue. I will update patch soon. I'm planning to include this in 3.4.9 release and please see the 3.4.9 release discussion

          Show
          rakeshr Rakesh R added a comment - Tsuyoshi Ozawa , thanks for the interest. I was busy with other things and will give priority to this issue. I will update patch soon. I'm planning to include this in 3.4.9 release and please see the 3.4.9 release discussion
          Hide
          rakeshr Rakesh R added a comment -

          Attached initial patch where I've used zkServer.isRunning() status to check whether the server is up and running. Appreciate your help in reviews. Thanks!

          Show
          rakeshr Rakesh R added a comment - Attached initial patch where I've used zkServer.isRunning() status to check whether the server is up and running. Appreciate your help in reviews. Thanks!
          Hide
          rakeshr Rakesh R added a comment -

          fyi: The patch got a green build_3284 , unfortunately there is a Permission denied issue in the build and due to that its not commenting to the jira. Anyone has idea about this problem?

          [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/test/bin/test-patch.sh: line 558: /home/jenkins/tools/jiracli/latest/jira: Permission denied
               [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/test/bin/test-patch.sh: line 559: /home/jenkins/tools/jiracli/latest/jira: Permission denied
          
          Show
          rakeshr Rakesh R added a comment - fyi: The patch got a green build_3284 , unfortunately there is a Permission denied issue in the build and due to that its not commenting to the jira. Anyone has idea about this problem? [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/test/bin/test-patch.sh: line 558: /home/jenkins/tools/jiracli/latest/jira: Permission denied [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/java/test/bin/test-patch.sh: line 559: /home/jenkins/tools/jiracli/latest/jira: Permission denied
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12819926/ZOOKEEPER-2383.patch
          against trunk revision 1754188.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3290//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3290//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3290//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12819926/ZOOKEEPER-2383.patch against trunk revision 1754188. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3290//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3290//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3290//console This message is automatically generated.
          Hide
          suda Akihiro Suda added a comment -

          +1 (non-binding).

          Locally tested ZooKeeperServerStartupTest 300 times and it passed.

          Show
          suda Akihiro Suda added a comment - +1 (non-binding). Locally tested ZooKeeperServerStartupTest 300 times and it passed.
          Hide
          rakeshr Rakesh R added a comment -

          Thank you Akihiro Suda for the feedback.

          Show
          rakeshr Rakesh R added a comment - Thank you Akihiro Suda for the feedback.
          Hide
          steve_rowe Steve Rowe added a comment -

          Rakesh R, looks like your patch is against trunk - if you make a branch-3.4 version of the patch, I'll run Solr's tests using a build off that branch (I'm assuming the next Solr ZK dep version will be 3.4.9).

          Show
          steve_rowe Steve Rowe added a comment - Rakesh R , looks like your patch is against trunk - if you make a branch-3.4 version of the patch, I'll run Solr's tests using a build off that branch (I'm assuming the next Solr ZK dep version will be 3.4.9).
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Steve Rowe. I'm attaching another patch based on br-3.4, would be great to see your feedback on this.

          Note: Jenkins will pick this ZK-2383-br-3-4 patch and will fail while applying against trunk code, please ignore it.

          Show
          rakeshr Rakesh R added a comment - Thanks Steve Rowe . I'm attaching another patch based on br-3.4, would be great to see your feedback on this. Note: Jenkins will pick this ZK-2383-br-3-4 patch and will fail while applying against trunk code, please ignore it.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12821043/ZOOKEEPER-2383-br-3-4.patch
          against trunk revision 1754188.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3310//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12821043/ZOOKEEPER-2383-br-3-4.patch against trunk revision 1754188. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3310//console This message is automatically generated.
          Hide
          steve_rowe Steve Rowe added a comment -

          Rakesh R, I patched branch-3.4 at revision 1754563 with your ZOOKEEPER-2383-br-3-4.patch, applied my SOLR-8724 patch with a small change to account for ZOOKEEPER-2141, and changed the depended-on ZK version from 3.4.8 to 3.4.9-SNAPSHOT (the version built off branch-3.4 with your patch), and Solr tests pass, so +1 from me to include your patch in the 3.4.9 release.

          Show
          steve_rowe Steve Rowe added a comment - Rakesh R , I patched branch-3.4 at revision 1754563 with your ZOOKEEPER-2383 -br-3-4.patch , applied my SOLR-8724 patch with a small change to account for ZOOKEEPER-2141 , and changed the depended-on ZK version from 3.4.8 to 3.4.9-SNAPSHOT (the version built off branch-3.4 with your patch), and Solr tests pass, so +1 from me to include your patch in the 3.4.9 release.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks a lot Steve Rowe for the feedback.

          Flavio Junqueira, Patrick Hunt, Raul Gutierrez Segales, do you have some cycles to review this. Thanks!

          Show
          rakeshr Rakesh R added a comment - Thanks a lot Steve Rowe for the feedback. Flavio Junqueira , Patrick Hunt , Raul Gutierrez Segales , do you have some cycles to review this. Thanks!
          Hide
          rgs Raul Gutierrez Segales added a comment -

          Rakesh R: looks good to me, one nit though. In testClientConnectionRequestDuringStartup:

          +        CountdownWatcher watcher = new CountdownWatcher();
          +        new ZooKeeper(HOSTPORT, ClientBase.CONNECTION_TIMEOUT, watcher);
          

          lets assign the ZooKeeper object to a var and explicitly clean it up when done. Other than that, +1.

          Happy to merge it after that. Thanks Rakesh R!

          Show
          rgs Raul Gutierrez Segales added a comment - Rakesh R : looks good to me, one nit though. In testClientConnectionRequestDuringStartup: + CountdownWatcher watcher = new CountdownWatcher(); + new ZooKeeper(HOSTPORT, ClientBase.CONNECTION_TIMEOUT, watcher); lets assign the ZooKeeper object to a var and explicitly clean it up when done. Other than that, +1. Happy to merge it after that. Thanks Rakesh R !
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Raul Gutierrez Segales. Attached new patch addressing the comments.

          Show
          rakeshr Rakesh R added a comment - Thanks Raul Gutierrez Segales . Attached new patch addressing the comments.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12822237/ZOOKEEPER-2383.patch
          against trunk revision 1755100.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3324//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3324//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3324//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12822237/ZOOKEEPER-2383.patch against trunk revision 1755100. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3324//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3324//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3324//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12822237/ZOOKEEPER-2383.patch
          against trunk revision 1755100.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3325//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3325//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3325//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12822237/ZOOKEEPER-2383.patch against trunk revision 1755100. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3325//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3325//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3325//console This message is automatically generated.
          Hide
          hanm Michael Han added a comment -

          ZOOKEEPER-2383-br-3-4.patch may also be updated with this fix as well.

          Show
          hanm Michael Han added a comment - ZOOKEEPER-2383 -br-3-4.patch may also be updated with this fix as well.
          Hide
          hanm Michael Han added a comment -

          One small suggestion - the condition of

           zkServer == null || !zkServer.isRunning() 

          could be refactored into a dedicated method such as

           boolean isZKServerRunning(ZooKeeperServer zkServer) { return zkServer != null && zkServer.isRunning(); }

          , which centralized such logic and make it reusable later. We could put it in Command.java or ZKUtil.java maybe?

          Show
          hanm Michael Han added a comment - One small suggestion - the condition of zkServer == null || !zkServer.isRunning() could be refactored into a dedicated method such as boolean isZKServerRunning(ZooKeeperServer zkServer) { return zkServer != null && zkServer.isRunning(); } , which centralized such logic and make it reusable later. We could put it in Command.java or ZKUtil.java maybe?
          Hide
          rakeshr Rakesh R added a comment - - edited

          Thanks Michael Han for the reviews. It looks like ZKUtil.java has ZooKeeper client related operations. Attached new patch, where I've tried keeping isZKServerRunning() in AbstractFourLetterCommand.java. Does this sound good to you?

          Show
          rakeshr Rakesh R added a comment - - edited Thanks Michael Han for the reviews. It looks like ZKUtil.java has ZooKeeper client related operations. Attached new patch, where I've tried keeping isZKServerRunning() in AbstractFourLetterCommand.java . Does this sound good to you?
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12822349/ZOOKEEPER-2383.patch
          against trunk revision 1755100.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3327//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3327//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3327//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12822349/ZOOKEEPER-2383.patch against trunk revision 1755100. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3327//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3327//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3327//console This message is automatically generated.
          Hide
          hanm Michael Han added a comment -

          Thanks Rakesh for quick reply - looks good to me.

          Show
          hanm Michael Han added a comment - Thanks Rakesh for quick reply - looks good to me.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12822349/ZOOKEEPER-2383.patch
          against trunk revision 1755379.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3328//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3328//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3328//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12822349/ZOOKEEPER-2383.patch against trunk revision 1755379. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3328//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3328//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3328//console This message is automatically generated.
          Hide
          rakeshr Rakesh R added a comment -

          Raul Gutierrez Segales, would you take another look at the latest patch and help me pushing this in. Thanks!

          Show
          rakeshr Rakesh R added a comment - Raul Gutierrez Segales , would you take another look at the latest patch and help me pushing this in. Thanks!
          Hide
          phunt Patrick Hunt added a comment -

          Raul Gutierrez Segales possible for you take another look?

          Show
          phunt Patrick Hunt added a comment - Raul Gutierrez Segales possible for you take another look?
          Hide
          rakeshr Rakesh R added a comment -

          I am moving this out to 3.4.10 for now. Please feel free to discuss the target version, Thanks!

          Show
          rakeshr Rakesh R added a comment - I am moving this out to 3.4.10 for now. Please feel free to discuss the target version, Thanks!
          Hide
          rakeshr Rakesh R added a comment -

          I've got +1(non-binding) from Steve Rowe and Michael Han.

          Hi Flavio Junqueira, Raul Gutierrez Segales, do you have some cycles to review the proposed patch considering the bug priority. Thanks!

          Show
          rakeshr Rakesh R added a comment - I've got +1(non-binding) from Steve Rowe and Michael Han . Hi Flavio Junqueira , Raul Gutierrez Segales , do you have some cycles to review the proposed patch considering the bug priority. Thanks!
          Hide
          fpj Flavio Junqueira added a comment -

          Rakesh R I have checked the 3.4 patch. In NettyServerCnxn, it looks like we are only updating the code for 4lws. Don't we have to also update it here:

                                  if (zks == null) {
                                      throw new IOException("ZK down");
                                  }
          

          This is in {{NettyServerCnxn.receiveMessage}.

          About the test case:

          1. I ran it with and without the changes. With the changes, it works fine. Without the changes, it hangs forever. I noticed in the logs that it gets:
          java.lang.AssertionError: Since zk server is not started createsession method to be invoked
          

          but it never exists as the client keeps trying to connect. It sounds like some thread is hanging and not letting the test framework exit.

          1. This sentence doesn't make much sense to me: Since zk server is not started createsession method to be invoked
          2. Please reduce the test case timeout to no longer than 30s.
          Show
          fpj Flavio Junqueira added a comment - Rakesh R I have checked the 3.4 patch. In NettyServerCnxn , it looks like we are only updating the code for 4lws. Don't we have to also update it here: if (zks == null) { throw new IOException("ZK down"); } This is in {{NettyServerCnxn.receiveMessage}. About the test case: I ran it with and without the changes. With the changes, it works fine. Without the changes, it hangs forever. I noticed in the logs that it gets: java.lang.AssertionError: Since zk server is not started createsession method to be invoked but it never exists as the client keeps trying to connect. It sounds like some thread is hanging and not letting the test framework exit. This sentence doesn't make much sense to me: Since zk server is not started createsession method to be invoked Please reduce the test case timeout to no longer than 30s.
          Hide
          rakeshr Rakesh R added a comment - - edited

          Thanks Flavio Junqueira for the comments.

          In NettyServerCnxn, it looks like we are only updating the code for 4lws. Don't we have to also update it here:

          I think, I have handled this case and the following condition present in ZOOKEEPER-2383-br-3-4.patch. Am I missing any other case apart from the below line of code.

          @@ -735,7 +735,7 @@
                                   bb.flip();
           
                                   ZooKeeperServer zks = this.zkServer;
          -                        if (zks == null) {
          +                        if (zks == null || !zks.isRunning()) {
                                       throw new IOException("ZK down");
                                   }
                                   if (initialized) {
          

          but it never exists as the client keeps trying to connect. It sounds like some thread is hanging and not letting the test framework exit.

          Without patch it fails at simplezks.waitForStartupInvocation(10). On the other side SimpleZooKeeperServer#startup thread is waiting at startupDelayLatch.await() and this is causing an indefinite wait. How about adding the countdown logic in finally so that it will proceed:

          	        try {
                      Assert.assertFalse(
                              "Should fail to create zk client session as server is not fully started",
                              simplezks.waitForSessionCreation(10));
                  } finally {
                      LOG.info(
                              "Decrements the count of the latch, so that server will proceed with startup");
                      startupDelayLatch.countDown();
                  }
          

          This sentence doesn't make much sense to me: Since zk server is not started createsession method to be invoked

          I will modify this to "Should fail to create zk client session as server is not fully started"

          Please reduce the test case timeout to no longer than 30s.

          Agreed.

          FYI, apart from the above, I had fixed Michael's comment and Raul's comment in trunk patch, need to rebase branch-3-4 patch to incorporate these comments.

          Show
          rakeshr Rakesh R added a comment - - edited Thanks Flavio Junqueira for the comments. In NettyServerCnxn, it looks like we are only updating the code for 4lws. Don't we have to also update it here: I think, I have handled this case and the following condition present in ZOOKEEPER-2383-br-3-4.patch . Am I missing any other case apart from the below line of code. @@ -735,7 +735,7 @@ bb.flip(); ZooKeeperServer zks = this .zkServer; - if (zks == null ) { + if (zks == null || !zks.isRunning()) { throw new IOException( "ZK down" ); } if (initialized) { but it never exists as the client keeps trying to connect. It sounds like some thread is hanging and not letting the test framework exit. Without patch it fails at simplezks.waitForStartupInvocation(10) . On the other side SimpleZooKeeperServer#startup thread is waiting at startupDelayLatch.await() and this is causing an indefinite wait. How about adding the countdown logic in finally so that it will proceed: try { Assert.assertFalse( "Should fail to create zk client session as server is not fully started" , simplezks.waitForSessionCreation(10)); } finally { LOG.info( "Decrements the count of the latch, so that server will proceed with startup" ); startupDelayLatch.countDown(); } This sentence doesn't make much sense to me: Since zk server is not started createsession method to be invoked I will modify this to "Should fail to create zk client session as server is not fully started" Please reduce the test case timeout to no longer than 30s. Agreed. FYI, apart from the above, I had fixed Michael's comment and Raul's comment in trunk patch, need to rebase branch-3-4 patch to incorporate these comments.
          Hide
          fpj Flavio Junqueira added a comment -

          Rakesh R the try/finally suggestion sounds good.

          I have a couple more comments that I'm not sure we can address, but I have have them written down at least:

          1. It sounds like you have covered all necessary zkServer == null cases, but I'm concerned we might have missed any, I'm not sure there is a way of guaranteeing and the only course of action is visual inspection. It'd be much better if we could sort out the problem in the startup call instead of relying on the methods accessing the zkServer to check whether the server is running.
            # The test case isn't covering the whole modified code. In particular, it is not covering the 4lw changes this patch is making.
          Show
          fpj Flavio Junqueira added a comment - Rakesh R the try/finally suggestion sounds good. I have a couple more comments that I'm not sure we can address, but I have have them written down at least: It sounds like you have covered all necessary zkServer == null cases, but I'm concerned we might have missed any, I'm not sure there is a way of guaranteeing and the only course of action is visual inspection. It'd be much better if we could sort out the problem in the startup call instead of relying on the methods accessing the zkServer to check whether the server is running. # The test case isn't covering the whole modified code. In particular, it is not covering the 4lw changes this patch is making.
          Hide
          fpj Flavio Junqueira added a comment -

          Actually, what prevents us from doing this:

          --- a/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
          +++ b/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
          @@ -113,10 +113,10 @@ public void start() {
               @Override
               public void startup(ZooKeeperServer zks) throws IOException,
                       InterruptedException {
          -        start();
                   setZooKeeperServer(zks);
                   zks.startdata();
                   zks.startup();
          +        start();
               }
          

          Interestingly, the tests pass for me with this change, so if it doesn't work, then we have at least a test coverage problem.

          Show
          fpj Flavio Junqueira added a comment - Actually, what prevents us from doing this: --- a/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java +++ b/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java @@ -113,10 +113,10 @@ public void start() { @Override public void startup(ZooKeeperServer zks) throws IOException, InterruptedException { - start(); setZooKeeperServer(zks); zks.startdata(); zks.startup(); + start(); } Interestingly, the tests pass for me with this change, so if it doesn't work, then we have at least a test coverage problem.
          Hide
          rakeshr Rakesh R added a comment -

          The test case isn't covering the whole modified code. In particular, it is not covering the 4lw changes this patch is making.

          Agreed, would need to add more test cases covering 4lw and netty server.

          Actually, what prevents us from doing this:

          Its an interesting thought to view the problem from different angle and frame a simple solution. I could see the following changes will happen if we move #start() at the end.

          1. If someone sends 4lw command to the server during startup, zk server won't respond to it and will not print message "This ZooKeeper instance is not currently serving requests".
          2. There is a change in the client and server side logging.
            Before:
            2016-09-20 22:17:33,542 [myid:127.0.0.1:11222] - INFO  [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1113] - Opening socket connection to server 127.0.0.1/127.0.0.1:11222. Will not attempt to authenticate using SASL (unknown error)
            2016-09-20 22:17:33,548 [myid:] - INFO  [NIOServerCxnFactory.AcceptThread:0.0.0.0/0.0.0.0:11222:NIOServerCnxnFactory$AcceptThread@296] - Accepted socket connection from /127.0.0.1:12510
            2016-09-20 22:17:33,548 [myid:127.0.0.1:11222] - INFO  [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@948] - Socket connection established, initiating session, client: /127.0.0.1:12510, server: 127.0.0.1/127.0.0.1:11222
            2016-09-20 22:17:33,563 [myid:] - WARN  [NIOWorkerThread-1:NIOServerCnxn@369] - Exception causing close of session 0x0: ZooKeeperServer not running
            2016-09-20 22:17:33,564 [myid:] - INFO  [NIOWorkerThread-1:NIOServerCnxn@607] - Closed socket connection for client /127.0.0.1:12510 (no session established for client)
            2016-09-20 22:17:33,564 [myid:127.0.0.1:11222] - INFO  [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1231] - Unable to read additional data from server sessionid 0x0, likely server has closed socket, closing socket connection and attempting reconnect
            

            After:

            2016-09-20 22:14:15,309 [myid:127.0.0.1:11222] - INFO  [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1113] - Opening socket connection to server 127.0.0.1/127.0.0.1:11222. Will not attempt to authenticate using SASL (unknown error)
            2016-09-20 22:14:15,312 [myid:127.0.0.1:11222] - INFO  [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@948] - Socket connection established, initiating session, client: /127.0.0.1:12418, server: 127.0.0.1/127.0.0.1:11222
            2016-09-20 22:14:45,313 [myid:127.0.0.1:11222] - WARN  [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1181] - Client session timed out, have not heard from server in 30001ms for sessionid 0x0
            

          Ideally server startup won't take too much time, only exceptional case is zks#loadData() is too large. I'm not aware about the use case of 4lws during startup, do anyone expect quick output shows server not running rather than connection timeout?

          Show
          rakeshr Rakesh R added a comment - The test case isn't covering the whole modified code. In particular, it is not covering the 4lw changes this patch is making. Agreed, would need to add more test cases covering 4lw and netty server. Actually, what prevents us from doing this: Its an interesting thought to view the problem from different angle and frame a simple solution. I could see the following changes will happen if we move #start() at the end. If someone sends 4lw command to the server during startup, zk server won't respond to it and will not print message "This ZooKeeper instance is not currently serving requests" . There is a change in the client and server side logging. Before: 2016-09-20 22:17:33,542 [myid:127.0.0.1:11222] - INFO [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1113] - Opening socket connection to server 127.0.0.1/127.0.0.1:11222. Will not attempt to authenticate using SASL (unknown error) 2016-09-20 22:17:33,548 [myid:] - INFO [NIOServerCxnFactory.AcceptThread:0.0.0.0/0.0.0.0:11222:NIOServerCnxnFactory$AcceptThread@296] - Accepted socket connection from /127.0.0.1:12510 2016-09-20 22:17:33,548 [myid:127.0.0.1:11222] - INFO [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@948] - Socket connection established, initiating session, client: /127.0.0.1:12510, server: 127.0.0.1/127.0.0.1:11222 2016-09-20 22:17:33,563 [myid:] - WARN [NIOWorkerThread-1:NIOServerCnxn@369] - Exception causing close of session 0x0: ZooKeeperServer not running 2016-09-20 22:17:33,564 [myid:] - INFO [NIOWorkerThread-1:NIOServerCnxn@607] - Closed socket connection for client /127.0.0.1:12510 (no session established for client) 2016-09-20 22:17:33,564 [myid:127.0.0.1:11222] - INFO [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1231] - Unable to read additional data from server sessionid 0x0, likely server has closed socket, closing socket connection and attempting reconnect After: 2016-09-20 22:14:15,309 [myid:127.0.0.1:11222] - INFO [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1113] - Opening socket connection to server 127.0.0.1/127.0.0.1:11222. Will not attempt to authenticate using SASL (unknown error) 2016-09-20 22:14:15,312 [myid:127.0.0.1:11222] - INFO [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@948] - Socket connection established, initiating session, client: /127.0.0.1:12418, server: 127.0.0.1/127.0.0.1:11222 2016-09-20 22:14:45,313 [myid:127.0.0.1:11222] - WARN [main-SendThread(127.0.0.1:11222):ClientCnxn$SendThread@1181] - Client session timed out, have not heard from server in 30001ms for sessionid 0x0 Ideally server startup won't take too much time, only exceptional case is zks#loadData() is too large. I'm not aware about the use case of 4lws during startup, do anyone expect quick output shows server not running rather than connection timeout?
          Hide
          rakeshr Rakesh R added a comment -

          Flavio Junqueira, any thoughts about the above points? Thanks!

          Show
          rakeshr Rakesh R added a comment - Flavio Junqueira , any thoughts about the above points? Thanks!
          Hide
          rakeshr Rakesh R added a comment -

          Will this proposed fix induce any other component integration test failures. Would be great to see your view on the above analysis. Thanks!

          Show
          rakeshr Rakesh R added a comment - Will this proposed fix induce any other component integration test failures. Would be great to see your view on the above analysis. Thanks!
          Hide
          fpj Flavio Junqueira added a comment -

          Moving the start() call to the end of the startup() method will cause the change of behavior you point out, that's correct, we shouldn't do it.

          Applying the test alone without the other changes still makes the test run forever. We need to fix it.

          There are a few format suggestions I wanted to make, perhaps it is better to have a pull request for this.

          Show
          fpj Flavio Junqueira added a comment - Moving the start() call to the end of the startup() method will cause the change of behavior you point out, that's correct, we shouldn't do it. Applying the test alone without the other changes still makes the test run forever. We need to fix it. There are a few format suggestions I wanted to make, perhaps it is better to have a pull request for this.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Flavio Junqueira. OK, I'll modify the test case and create pull request.

          Show
          rakeshr Rakesh R added a comment - Thanks Flavio Junqueira . OK, I'll modify the test case and create pull request.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user rakeshadr opened a pull request:

          https://github.com/apache/zookeeper/pull/101

          ZOOKEEPER-2383

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/rakeshadr/zookeeper-1 ZK-2383

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/zookeeper/pull/101.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #101


          commit 401ce15e7cd1eeb0e8cef367bff53c1054ea17df
          Author: Rakesh Radhakrishnan <rakeshr@apache.org>
          Date: 2016-11-06T16:24:53Z

          ZOOKEEPER-2383


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user rakeshadr opened a pull request: https://github.com/apache/zookeeper/pull/101 ZOOKEEPER-2383 You can merge this pull request into a Git repository by running: $ git pull https://github.com/rakeshadr/zookeeper-1 ZK-2383 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/101.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #101 commit 401ce15e7cd1eeb0e8cef367bff53c1054ea17df Author: Rakesh Radhakrishnan <rakeshr@apache.org> Date: 2016-11-06T16:24:53Z ZOOKEEPER-2383
          Hide
          rakeshr Rakesh R added a comment -

          Attached patch including test cases to verify the changs w.r.t NIOServerCnxn, NettyServerCnxn, flw commands.

          Hi Flavio Junqueira, Created PR https://github.com/apache/zookeeper/pull/101, please take look at this. Thanks!

          Show
          rakeshr Rakesh R added a comment - Attached patch including test cases to verify the changs w.r.t NIOServerCnxn , NettyServerCnxn , flw commands . Hi Flavio Junqueira , Created PR https://github.com/apache/zookeeper/pull/101 , please take look at this. Thanks!
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12837652/ZOOKEEPER-2383.patch
          against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3526//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3526//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3526//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12837652/ZOOKEEPER-2383.patch against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3526//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3526//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3526//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12837652/ZOOKEEPER-2383.patch
          against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3527//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3527//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3527//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12837652/ZOOKEEPER-2383.patch against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3527//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3527//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3527//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/46//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/46//console This message is automatically generated.
          Hide
          rakeshr Rakesh R added a comment -

          Hi Flavio Junqueira, would be great to see your feedback on the proposed code changes, do you have some cycles to review this. Thanks!

          Show
          rakeshr Rakesh R added a comment - Hi Flavio Junqueira , would be great to see your feedback on the proposed code changes , do you have some cycles to review this. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/101#discussion_r92712695

          — Diff: src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java —
          @@ -0,0 +1,266 @@
          +/**
          + * 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.zookeeper.server;
          +
          +import static org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
          +
          +import java.io.File;
          +import java.io.IOException;
          +import java.util.concurrent.CountDownLatch;
          +import java.util.concurrent.TimeUnit;
          +
          +import org.apache.zookeeper.PortAssignment;
          +import org.apache.zookeeper.ZKTestCase;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.common.X509Exception.SSLContextException;
          +import org.apache.zookeeper.test.ClientBase;
          +import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
          +import org.junit.After;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +/**
          + * This class tests the startup behavior of ZooKeeper server.
          + */
          +public class ZooKeeperServerStartupTest extends ZKTestCase {
          + private static final Logger LOG = LoggerFactory
          + .getLogger(ZooKeeperServerStartupTest.class);
          + private static int PORT = PortAssignment.unique();
          + private static String HOST = "127.0.0.1";
          + private static String HOSTPORT = HOST + ":" + PORT;
          + private static final String ZK_NOT_SERVING = "This ZooKeeper instance is not currently serving requests";
          +
          + private ServerCnxnFactory servcnxnf;
          + private ZooKeeperServer zks;
          + private File tmpDir;
          + private CountDownLatch startupDelayLatch = new CountDownLatch(1);
          +
          + @After
          + public void teardown() throws Exception {
          + // count down to avoid infinite blocking call due to this latch, if
          + // any.
          + startupDelayLatch.countDown();
          +
          + if (servcnxnf != null)

          { + servcnxnf.shutdown(); + }

          + if (zks != null)

          { + zks.shutdown(); + }

          + if (zks.getZKDatabase() != null)

          { + zks.getZKDatabase().close(); + }

          + ClientBase.recursiveDelete(tmpDir);
          + }
          +
          + /**
          + * Test case for
          + *

          {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}

          .
          + */
          + @Test(timeout = 30000)
          + public void testClientConnectionRequestDuringStartupWithNIOServerCnxn()
          + throws Exception

          { + tmpDir = ClientBase.createTmpDir(); + ClientBase.setupTestEnv(); + + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + }

          +
          + /**
          + * Test case for
          + *

          {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}

          .
          + */
          + @Test(timeout = 30000)
          + public void testClientConnectionRequestDuringStartupWithNettyServerCnxn()
          + throws Exception {
          + tmpDir = ClientBase.createTmpDir();
          + ClientBase.setupTestEnv();
          +
          + String originalServerCnxnFactory = System
          + .getProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
          + try

          { + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, + NettyServerCnxnFactory.class.getName()); + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + }

          finally {
          + // reset cnxn factory
          + if (originalServerCnxnFactory == null)

          { + System.clearProperty( + ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); + return; + }

          + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
          + originalServerCnxnFactory);
          + }
          + }
          +
          + /**
          + * Test case for
          + *

          {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}

          .
          + */
          + @Test(timeout = 30000)
          + public void testFourLetterWords() throws Exception

          { + startSimpleZKServer(startupDelayLatch); + verify("conf", ZK_NOT_SERVING); + verify("crst", ZK_NOT_SERVING); + verify("cons", ZK_NOT_SERVING); + verify("dirs", ZK_NOT_SERVING); + verify("dump", ZK_NOT_SERVING); + verify("mntr", ZK_NOT_SERVING); + verify("stat", ZK_NOT_SERVING); + verify("srst", ZK_NOT_SERVING); + verify("wchp", ZK_NOT_SERVING); + verify("wchc", ZK_NOT_SERVING); + verify("wchs", ZK_NOT_SERVING); + verify("isro", "null"); + }

          +
          + private void verify(String cmd, String expected)
          + throws IOException, SSLContextException

          { + String resp = sendRequest(cmd); + LOG.info("cmd " + cmd + " expected " + expected + " got " + resp); + Assert.assertTrue("Unexpected response", resp.contains(expected)); + }

          +
          + private String sendRequest(String cmd)
          + throws IOException, SSLContextException

          { + return send4LetterWord(HOST, PORT, cmd); + }

          +
          + private void startSimpleZKServer(CountDownLatch startupDelayLatch)
          + throws IOException {
          + zks = new SimpleZooKeeperServer(tmpDir, tmpDir, 3000,
          + startupDelayLatch);
          + SyncRequestProcessor.setSnapCount(100);
          + final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
          +
          + servcnxnf = ServerCnxnFactory.createFactory(PORT, -1);
          + Thread startupThread = new Thread() {
          + public void run() {
          + try

          { + servcnxnf.startup(zks); + }

          catch (IOException | InterruptedException e) {
          — End diff –

          We can't have a multi-catch with Java 6, which is the earliest version we say we support in the docs.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/101#discussion_r92712695 — Diff: src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java — @@ -0,0 +1,266 @@ +/** + * 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.zookeeper.server; + +import static org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord; + +import java.io.File; +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import org.apache.zookeeper.PortAssignment; +import org.apache.zookeeper.ZKTestCase; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.common.X509Exception.SSLContextException; +import org.apache.zookeeper.test.ClientBase; +import org.apache.zookeeper.test.ClientBase.CountdownWatcher; +import org.junit.After; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This class tests the startup behavior of ZooKeeper server. + */ +public class ZooKeeperServerStartupTest extends ZKTestCase { + private static final Logger LOG = LoggerFactory + .getLogger(ZooKeeperServerStartupTest.class); + private static int PORT = PortAssignment.unique(); + private static String HOST = "127.0.0.1"; + private static String HOSTPORT = HOST + ":" + PORT; + private static final String ZK_NOT_SERVING = "This ZooKeeper instance is not currently serving requests"; + + private ServerCnxnFactory servcnxnf; + private ZooKeeperServer zks; + private File tmpDir; + private CountDownLatch startupDelayLatch = new CountDownLatch(1); + + @After + public void teardown() throws Exception { + // count down to avoid infinite blocking call due to this latch, if + // any. + startupDelayLatch.countDown(); + + if (servcnxnf != null) { + servcnxnf.shutdown(); + } + if (zks != null) { + zks.shutdown(); + } + if (zks.getZKDatabase() != null) { + zks.getZKDatabase().close(); + } + ClientBase.recursiveDelete(tmpDir); + } + + /** + * Test case for + * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383} . + */ + @Test(timeout = 30000) + public void testClientConnectionRequestDuringStartupWithNIOServerCnxn() + throws Exception { + tmpDir = ClientBase.createTmpDir(); + ClientBase.setupTestEnv(); + + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + } + + /** + * Test case for + * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383} . + */ + @Test(timeout = 30000) + public void testClientConnectionRequestDuringStartupWithNettyServerCnxn() + throws Exception { + tmpDir = ClientBase.createTmpDir(); + ClientBase.setupTestEnv(); + + String originalServerCnxnFactory = System + .getProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); + try { + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, + NettyServerCnxnFactory.class.getName()); + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + } finally { + // reset cnxn factory + if (originalServerCnxnFactory == null) { + System.clearProperty( + ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); + return; + } + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, + originalServerCnxnFactory); + } + } + + /** + * Test case for + * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383} . + */ + @Test(timeout = 30000) + public void testFourLetterWords() throws Exception { + startSimpleZKServer(startupDelayLatch); + verify("conf", ZK_NOT_SERVING); + verify("crst", ZK_NOT_SERVING); + verify("cons", ZK_NOT_SERVING); + verify("dirs", ZK_NOT_SERVING); + verify("dump", ZK_NOT_SERVING); + verify("mntr", ZK_NOT_SERVING); + verify("stat", ZK_NOT_SERVING); + verify("srst", ZK_NOT_SERVING); + verify("wchp", ZK_NOT_SERVING); + verify("wchc", ZK_NOT_SERVING); + verify("wchs", ZK_NOT_SERVING); + verify("isro", "null"); + } + + private void verify(String cmd, String expected) + throws IOException, SSLContextException { + String resp = sendRequest(cmd); + LOG.info("cmd " + cmd + " expected " + expected + " got " + resp); + Assert.assertTrue("Unexpected response", resp.contains(expected)); + } + + private String sendRequest(String cmd) + throws IOException, SSLContextException { + return send4LetterWord(HOST, PORT, cmd); + } + + private void startSimpleZKServer(CountDownLatch startupDelayLatch) + throws IOException { + zks = new SimpleZooKeeperServer(tmpDir, tmpDir, 3000, + startupDelayLatch); + SyncRequestProcessor.setSnapCount(100); + final int PORT = Integer.parseInt(HOSTPORT.split(":") [1] ); + + servcnxnf = ServerCnxnFactory.createFactory(PORT, -1); + Thread startupThread = new Thread() { + public void run() { + try { + servcnxnf.startup(zks); + } catch (IOException | InterruptedException e) { — End diff – We can't have a multi-catch with Java 6, which is the earliest version we say we support in the docs.
          Hide
          fpj Flavio Junqueira added a comment -

          There is a compilation issue in the 3.4 branch, I left a comment in the pull request:

          compile-test:
              [mkdir] Created dir: /Users/fpj/code/github/zookeeper/build/test/classes
              [javac] Compiling 145 source files to /Users/fpj/code/github/zookeeper/build/test/classes
              [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.6
              [javac] /Users/fpj/code/github/zookeeper/src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java:209: error: multi-catch statement is not supported in -source 1.6
              [javac]                 } catch (IOException | InterruptedException e) {
              [javac]                                      ^
              [javac]   (use -source 7 or higher to enable multi-catch statement)
              [javac] 1 error
          
          Show
          fpj Flavio Junqueira added a comment - There is a compilation issue in the 3.4 branch, I left a comment in the pull request: compile-test: [mkdir] Created dir: /Users/fpj/code/github/zookeeper/build/test/classes [javac] Compiling 145 source files to /Users/fpj/code/github/zookeeper/build/test/classes [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.6 [javac] /Users/fpj/code/github/zookeeper/src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java:209: error: multi-catch statement is not supported in -source 1.6 [javac] } catch (IOException | InterruptedException e) { [javac] ^ [javac] (use -source 7 or higher to enable multi-catch statement) [javac] 1 error
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/101#discussion_r92785304

          — Diff: src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java —
          @@ -0,0 +1,266 @@
          +/**
          + * 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.zookeeper.server;
          +
          +import static org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
          +
          +import java.io.File;
          +import java.io.IOException;
          +import java.util.concurrent.CountDownLatch;
          +import java.util.concurrent.TimeUnit;
          +
          +import org.apache.zookeeper.PortAssignment;
          +import org.apache.zookeeper.ZKTestCase;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.common.X509Exception.SSLContextException;
          +import org.apache.zookeeper.test.ClientBase;
          +import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
          +import org.junit.After;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +/**
          + * This class tests the startup behavior of ZooKeeper server.
          + */
          +public class ZooKeeperServerStartupTest extends ZKTestCase {
          + private static final Logger LOG = LoggerFactory
          + .getLogger(ZooKeeperServerStartupTest.class);
          + private static int PORT = PortAssignment.unique();
          + private static String HOST = "127.0.0.1";
          + private static String HOSTPORT = HOST + ":" + PORT;
          + private static final String ZK_NOT_SERVING = "This ZooKeeper instance is not currently serving requests";
          +
          + private ServerCnxnFactory servcnxnf;
          + private ZooKeeperServer zks;
          + private File tmpDir;
          + private CountDownLatch startupDelayLatch = new CountDownLatch(1);
          +
          + @After
          + public void teardown() throws Exception {
          + // count down to avoid infinite blocking call due to this latch, if
          + // any.
          + startupDelayLatch.countDown();
          +
          + if (servcnxnf != null)

          { + servcnxnf.shutdown(); + }

          + if (zks != null)

          { + zks.shutdown(); + }

          + if (zks.getZKDatabase() != null)

          { + zks.getZKDatabase().close(); + }

          + ClientBase.recursiveDelete(tmpDir);
          + }
          +
          + /**
          + * Test case for
          + *

          {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}

          .
          + */
          + @Test(timeout = 30000)
          + public void testClientConnectionRequestDuringStartupWithNIOServerCnxn()
          + throws Exception

          { + tmpDir = ClientBase.createTmpDir(); + ClientBase.setupTestEnv(); + + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + }

          +
          + /**
          + * Test case for
          + *

          {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}

          .
          + */
          + @Test(timeout = 30000)
          + public void testClientConnectionRequestDuringStartupWithNettyServerCnxn()
          + throws Exception {
          + tmpDir = ClientBase.createTmpDir();
          + ClientBase.setupTestEnv();
          +
          + String originalServerCnxnFactory = System
          + .getProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
          + try

          { + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, + NettyServerCnxnFactory.class.getName()); + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + }

          finally {
          + // reset cnxn factory
          + if (originalServerCnxnFactory == null)

          { + System.clearProperty( + ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); + return; + }

          + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
          + originalServerCnxnFactory);
          + }
          + }
          +
          + /**
          + * Test case for
          + *

          {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}

          .
          + */
          + @Test(timeout = 30000)
          + public void testFourLetterWords() throws Exception

          { + startSimpleZKServer(startupDelayLatch); + verify("conf", ZK_NOT_SERVING); + verify("crst", ZK_NOT_SERVING); + verify("cons", ZK_NOT_SERVING); + verify("dirs", ZK_NOT_SERVING); + verify("dump", ZK_NOT_SERVING); + verify("mntr", ZK_NOT_SERVING); + verify("stat", ZK_NOT_SERVING); + verify("srst", ZK_NOT_SERVING); + verify("wchp", ZK_NOT_SERVING); + verify("wchc", ZK_NOT_SERVING); + verify("wchs", ZK_NOT_SERVING); + verify("isro", "null"); + }

          +
          + private void verify(String cmd, String expected)
          + throws IOException, SSLContextException

          { + String resp = sendRequest(cmd); + LOG.info("cmd " + cmd + " expected " + expected + " got " + resp); + Assert.assertTrue("Unexpected response", resp.contains(expected)); + }

          +
          + private String sendRequest(String cmd)
          + throws IOException, SSLContextException

          { + return send4LetterWord(HOST, PORT, cmd); + }

          +
          + private void startSimpleZKServer(CountDownLatch startupDelayLatch)
          + throws IOException {
          + zks = new SimpleZooKeeperServer(tmpDir, tmpDir, 3000,
          + startupDelayLatch);
          + SyncRequestProcessor.setSnapCount(100);
          + final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
          +
          + servcnxnf = ServerCnxnFactory.createFactory(PORT, -1);
          + Thread startupThread = new Thread() {
          + public void run() {
          + try

          { + servcnxnf.startup(zks); + }

          catch (IOException | InterruptedException e) {
          — End diff –

          Thank you @fpj . I've separated out the catch block and done another commit in PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/101#discussion_r92785304 — Diff: src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java — @@ -0,0 +1,266 @@ +/** + * 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.zookeeper.server; + +import static org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord; + +import java.io.File; +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import org.apache.zookeeper.PortAssignment; +import org.apache.zookeeper.ZKTestCase; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.common.X509Exception.SSLContextException; +import org.apache.zookeeper.test.ClientBase; +import org.apache.zookeeper.test.ClientBase.CountdownWatcher; +import org.junit.After; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This class tests the startup behavior of ZooKeeper server. + */ +public class ZooKeeperServerStartupTest extends ZKTestCase { + private static final Logger LOG = LoggerFactory + .getLogger(ZooKeeperServerStartupTest.class); + private static int PORT = PortAssignment.unique(); + private static String HOST = "127.0.0.1"; + private static String HOSTPORT = HOST + ":" + PORT; + private static final String ZK_NOT_SERVING = "This ZooKeeper instance is not currently serving requests"; + + private ServerCnxnFactory servcnxnf; + private ZooKeeperServer zks; + private File tmpDir; + private CountDownLatch startupDelayLatch = new CountDownLatch(1); + + @After + public void teardown() throws Exception { + // count down to avoid infinite blocking call due to this latch, if + // any. + startupDelayLatch.countDown(); + + if (servcnxnf != null) { + servcnxnf.shutdown(); + } + if (zks != null) { + zks.shutdown(); + } + if (zks.getZKDatabase() != null) { + zks.getZKDatabase().close(); + } + ClientBase.recursiveDelete(tmpDir); + } + + /** + * Test case for + * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383} . + */ + @Test(timeout = 30000) + public void testClientConnectionRequestDuringStartupWithNIOServerCnxn() + throws Exception { + tmpDir = ClientBase.createTmpDir(); + ClientBase.setupTestEnv(); + + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + } + + /** + * Test case for + * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383} . + */ + @Test(timeout = 30000) + public void testClientConnectionRequestDuringStartupWithNettyServerCnxn() + throws Exception { + tmpDir = ClientBase.createTmpDir(); + ClientBase.setupTestEnv(); + + String originalServerCnxnFactory = System + .getProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); + try { + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, + NettyServerCnxnFactory.class.getName()); + startSimpleZKServer(startupDelayLatch); + SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks; + Assert.assertTrue( + "Failed to invoke zks#startup() method during server startup", + simplezks.waitForStartupInvocation(10)); + + CountdownWatcher watcher = new CountdownWatcher(); + ZooKeeper zkClient = new ZooKeeper(HOSTPORT, + ClientBase.CONNECTION_TIMEOUT, watcher); + + Assert.assertFalse( + "Since server is not fully started, zks#createSession() shouldn't be invoked", + simplezks.waitForSessionCreation(5)); + + LOG.info( + "Decrements the count of the latch, so that server will proceed with startup"); + startupDelayLatch.countDown(); + + Assert.assertTrue("waiting for server being up ", ClientBase + .waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT)); + + Assert.assertTrue( + "Failed to invoke zks#createSession() method during client session creation", + simplezks.waitForSessionCreation(5)); + watcher.waitForConnected(ClientBase.CONNECTION_TIMEOUT); + zkClient.close(); + } finally { + // reset cnxn factory + if (originalServerCnxnFactory == null) { + System.clearProperty( + ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); + return; + } + System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, + originalServerCnxnFactory); + } + } + + /** + * Test case for + * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383} . + */ + @Test(timeout = 30000) + public void testFourLetterWords() throws Exception { + startSimpleZKServer(startupDelayLatch); + verify("conf", ZK_NOT_SERVING); + verify("crst", ZK_NOT_SERVING); + verify("cons", ZK_NOT_SERVING); + verify("dirs", ZK_NOT_SERVING); + verify("dump", ZK_NOT_SERVING); + verify("mntr", ZK_NOT_SERVING); + verify("stat", ZK_NOT_SERVING); + verify("srst", ZK_NOT_SERVING); + verify("wchp", ZK_NOT_SERVING); + verify("wchc", ZK_NOT_SERVING); + verify("wchs", ZK_NOT_SERVING); + verify("isro", "null"); + } + + private void verify(String cmd, String expected) + throws IOException, SSLContextException { + String resp = sendRequest(cmd); + LOG.info("cmd " + cmd + " expected " + expected + " got " + resp); + Assert.assertTrue("Unexpected response", resp.contains(expected)); + } + + private String sendRequest(String cmd) + throws IOException, SSLContextException { + return send4LetterWord(HOST, PORT, cmd); + } + + private void startSimpleZKServer(CountDownLatch startupDelayLatch) + throws IOException { + zks = new SimpleZooKeeperServer(tmpDir, tmpDir, 3000, + startupDelayLatch); + SyncRequestProcessor.setSnapCount(100); + final int PORT = Integer.parseInt(HOSTPORT.split(":") [1] ); + + servcnxnf = ServerCnxnFactory.createFactory(PORT, -1); + Thread startupThread = new Thread() { + public void run() { + try { + servcnxnf.startup(zks); + } catch (IOException | InterruptedException e) { — End diff – Thank you @fpj . I've separated out the catch block and done another commit in PR.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/130//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/130//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/130//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/130//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/130//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/130//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/129//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/129//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/129//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/129//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/129//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/129//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fpj commented on the issue:

          https://github.com/apache/zookeeper/pull/101

          This is including the commit I just did for ZOOKEEPER-761, could you rebase your branch, please, @rakeshadr ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/101 This is including the commit I just did for ZOOKEEPER-761 , could you rebase your branch, please, @rakeshadr ?
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/134//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/134//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/134//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/134//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/134//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/134//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr commented on the issue:

          https://github.com/apache/zookeeper/pull/101

          oops, I've rebased it and hope the PR is clean now. Please take a look at this when you get some time. Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on the issue: https://github.com/apache/zookeeper/pull/101 oops, I've rebased it and hope the PR is clean now. Please take a look at this when you get some time. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/zookeeper/pull/101

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/101
          Hide
          fpj Flavio Junqueira added a comment - - edited

          Rakesh R pull request #101 merged fine onto master, but not branch 3.5. I haven't tried branch 3.4.

          Show
          fpj Flavio Junqueira added a comment - - edited Rakesh R pull request #101 merged fine onto master, but not branch 3.5. I haven't tried branch 3.4.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build ZooKeeper-trunk #3204 (See https://builds.apache.org/job/ZooKeeper-trunk/3204/)
          ZOOKEEPER-2383: Startup race in ZooKeeperServer (fpj: rev eac693cc76a34f96b9116ef33d1e92af7129416d)

          • (edit) src/java/main/org/apache/zookeeper/server/command/AbstractFourLetterCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/DumpCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/StatResetCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/IsroCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/ConsCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/admin/Commands.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/ConfCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/WatchCommand.java
          • (add) src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java
          • (edit) src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java
          • (edit) src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/CnxnStatResetCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/DirsCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/StatCommand.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build ZooKeeper-trunk #3204 (See https://builds.apache.org/job/ZooKeeper-trunk/3204/ ) ZOOKEEPER-2383 : Startup race in ZooKeeperServer (fpj: rev eac693cc76a34f96b9116ef33d1e92af7129416d) (edit) src/java/main/org/apache/zookeeper/server/command/AbstractFourLetterCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/DumpCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/StatResetCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/IsroCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/ConsCommand.java (edit) src/java/main/org/apache/zookeeper/server/admin/Commands.java (edit) src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/ConfCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/WatchCommand.java (add) src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java (edit) src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java (edit) src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java (edit) src/java/main/org/apache/zookeeper/server/command/CnxnStatResetCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/DirsCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/StatCommand.java
          Hide
          rakeshr Rakesh R added a comment -

          Thank you Flavio Junqueira for the help in resolving this.

          pull request #101 merged fine onto master, but not branch 3.5. I haven't tried branch 3.4.

          I've tried cherry pick the commits to branch-3.5 locally and it worked. Is there anything specific to be done to branch 3.5?. But it requires a separate PR for branch 3.4 as the code is different from master branch, I'will create a PR for branch 3.4 shortly.

          Show
          rakeshr Rakesh R added a comment - Thank you Flavio Junqueira for the help in resolving this. pull request #101 merged fine onto master, but not branch 3.5. I haven't tried branch 3.4. I've tried cherry pick the commits to branch-3.5 locally and it worked. Is there anything specific to be done to branch 3.5 ?. But it requires a separate PR for branch 3.4 as the code is different from master branch, I'will create a PR for branch 3.4 shortly.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Michael Han for merging this to branch-3.5

          Show
          rakeshr Rakesh R added a comment - Thanks Michael Han for merging this to branch-3.5
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user rakeshadr opened a pull request:

          https://github.com/apache/zookeeper/pull/135

          ZOOKEEPER-2383: Startup race in ZooKeeperServer

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/rakeshadr/zookeeper-1 ZK-2383-br-3.4

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/zookeeper/pull/135.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #135


          commit 08d26512f131897f82b2566439c55c8fe539c4e5
          Author: Rakesh Radhakrishnan <rakeshr@apache.org>
          Date: 2016-12-23T16:54:12Z

          ZOOKEEPER-2383: Startup race in ZooKeeperServer


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user rakeshadr opened a pull request: https://github.com/apache/zookeeper/pull/135 ZOOKEEPER-2383 : Startup race in ZooKeeperServer You can merge this pull request into a Git repository by running: $ git pull https://github.com/rakeshadr/zookeeper-1 ZK-2383-br-3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/135.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #135 commit 08d26512f131897f82b2566439c55c8fe539c4e5 Author: Rakesh Radhakrishnan <rakeshr@apache.org> Date: 2016-12-23T16:54:12Z ZOOKEEPER-2383 : Startup race in ZooKeeperServer
          Hide
          rakeshr Rakesh R added a comment -

          PR_135 is created to merge the changes to branch-3.4. Please review this. Thanks!

          Show
          rakeshr Rakesh R added a comment - PR_135 is created to merge the changes to branch-3.4 . Please review this. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

          https://github.com/apache/zookeeper/pull/135

          @rakeshadr The test file is put in source folder (src/java/main/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java) - it should be put in test folder (src/java/test) right?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/135 @rakeshadr The test file is put in source folder (src/java/main/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java) - it should be put in test folder (src/java/test) right?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr commented on the issue:

          https://github.com/apache/zookeeper/pull/135

          Thanks @hanm for pointing out this. I've moved the test accordingly.

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on the issue: https://github.com/apache/zookeeper/pull/135 Thanks @hanm for pointing out this. I've moved the test accordingly.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

          https://github.com/apache/zookeeper/pull/135

          Thanks @rakeshadr , looks good. Merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/135 Thanks @rakeshadr , looks good. Merging.
          Hide
          hanm Michael Han added a comment -

          Issue resolved by pull request 135
          https://github.com/apache/zookeeper/pull/135

          Show
          hanm Michael Han added a comment - Issue resolved by pull request 135 https://github.com/apache/zookeeper/pull/135
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

          https://github.com/apache/zookeeper/pull/135

          @rakeshadr Could you close this PR since it has been merged in? Thanks.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/135 @rakeshadr Could you close this PR since it has been merged in? Thanks.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr closed the pull request at:

          https://github.com/apache/zookeeper/pull/135

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr closed the pull request at: https://github.com/apache/zookeeper/pull/135
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr commented on the issue:

          https://github.com/apache/zookeeper/pull/135

          Done!

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on the issue: https://github.com/apache/zookeeper/pull/135 Done!

            People

            • Assignee:
              rakeshr Rakesh R
              Reporter:
              steve_rowe Steve Rowe
            • Votes:
              2 Vote for this issue
              Watchers:
              17 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development