virajjasani commented on a change in pull request #1684: URL: https://github.com/apache/hbase/pull/1684#discussion_r422598671
########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java ########## @@ -151,19 +156,47 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception { hbaseCluster = new LocalHBaseCluster(htu.getConfiguration(), options.getNumMasters(), options.getNumRegionServers(), options.getMasterClass(), options.getRsClass()); final MasterThread masterThread = hbaseCluster.getMasters().get(0); + + final CompletableFuture<Void> shutdownFuture = CompletableFuture.runAsync(() -> { + // Switching to master registry exacerbated a race in the master bootstrap that can result + // in a lost shutdown command (HBASE-8422, HBASE-23836). The race is essentially because + // the server manager in HMaster is not initialized by the time shutdown() RPC (below) is + // made to the master. The suspected reason as to why it was uncommon before HBASE-18095 + // is because the connection creation with ZK registry is so slow that by then the server + // manager is usually init'ed in time for the RPC to be made. For now, adding an explicit + // wait() in the test, waiting for the server manager to become available. + final long timeout = TimeUnit.MINUTES.toMillis(10); + assertNotEquals("timeout waiting for server manager to become available.", -1, + htu.waitFor(timeout, () -> masterThread.getMaster().getServerManager() != null)); + + // Master has come up far enough that we can terminate it without creating a zombie. + final long result = htu.waitFor(timeout, 1000, () -> { + final Configuration conf = createResponsiveZkConfig(htu.getConfiguration()); + LOG.debug("Attempting to establish connection."); + final CompletableFuture<AsyncConnection> connFuture = + ConnectionFactory.createAsyncConnection(conf); + try (final AsyncConnection conn = connFuture.join()) { Review comment: since we want to use AsynAdmin with AsyncConnection, htu.getConnection() wouldn't work. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java ########## @@ -151,19 +156,47 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception { hbaseCluster = new LocalHBaseCluster(htu.getConfiguration(), options.getNumMasters(), options.getNumRegionServers(), options.getMasterClass(), options.getRsClass()); final MasterThread masterThread = hbaseCluster.getMasters().get(0); + + final CompletableFuture<Void> shutdownFuture = CompletableFuture.runAsync(() -> { + // Switching to master registry exacerbated a race in the master bootstrap that can result + // in a lost shutdown command (HBASE-8422, HBASE-23836). The race is essentially because + // the server manager in HMaster is not initialized by the time shutdown() RPC (below) is + // made to the master. The suspected reason as to why it was uncommon before HBASE-18095 + // is because the connection creation with ZK registry is so slow that by then the server + // manager is usually init'ed in time for the RPC to be made. For now, adding an explicit + // wait() in the test, waiting for the server manager to become available. + final long timeout = TimeUnit.MINUTES.toMillis(10); + assertNotEquals("timeout waiting for server manager to become available.", -1, + htu.waitFor(timeout, () -> masterThread.getMaster().getServerManager() != null)); + + // Master has come up far enough that we can terminate it without creating a zombie. + final long result = htu.waitFor(timeout, 1000, () -> { + final Configuration conf = createResponsiveZkConfig(htu.getConfiguration()); + LOG.debug("Attempting to establish connection."); + final CompletableFuture<AsyncConnection> connFuture = + ConnectionFactory.createAsyncConnection(conf); + try (final AsyncConnection conn = connFuture.join()) { + LOG.info("Sending shutdown RPC."); + try { + conn.getAdmin().shutdown().join(); + LOG.info("Shutdown RPC sent."); + return true; + } catch (CompletionException e) { + LOG.error("Failure sending shutdown RPC."); + } + } catch (IOException|CompletionException e) { + LOG.error("Failed to establish connection."); + } catch (Throwable e) { + LOG.error("Something unexpected happened.", e); + } + return false; + }); + assertNotEquals("Failed to issue shutdown RPC after " + Duration.ofMillis(timeout), + -1, result); + }); + masterThread.start(); Review comment: Oh yes, let me update. Thanks ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org