[GitHub] asfgit closed pull request #1577: DRILL-6912: NPE when other drillbit is already running

[GitBox <gi...@apache.org>]

asfgit closed pull request #1577: DRILL-6912: NPE when other drillbit is already running
URL: https://github.com/apache/drill/pull/1577
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java
index 4c595a2aa60..8d413977f5c 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java
@@ -98,7 +98,7 @@
   private DrillbitStateManager stateManager;
   private boolean quiescentMode;
   private boolean forcefulShutdown = false;
-  GracefulShutdownThread gracefulShutdownThread;
+  private GracefulShutdownThread gracefulShutdownThread;
   private boolean interruptPollShutdown = true;
 
   public void setQuiescentMode(boolean quiescentMode) {
@@ -196,6 +196,7 @@ public int getWebServerPort() {
   public void run() throws Exception {
     final Stopwatch w = Stopwatch.createStarted();
     logger.debug("Startup begun.");
+    gracefulShutdownThread = new GracefulShutdownThread(this, new StackTrace());
     coord.start(10000);
     stateManager.setState(DrillbitState.ONLINE);
     storeProvider.start();
@@ -222,7 +223,6 @@ public void run() throws Exception {
     drillbitContext.startRM();
 
     Runtime.getRuntime().addShutdownHook(new ShutdownThread(this, new StackTrace()));
-    gracefulShutdownThread = new GracefulShutdownThread(this, new StackTrace());
     gracefulShutdownThread.start();
     logger.info("Startup completed ({} ms).", w.elapsed(TimeUnit.MILLISECONDS));
   }
@@ -470,6 +470,11 @@ public DrillbitContext getContext() {
     return manager.getContext();
   }
 
+  @VisibleForTesting
+  public GracefulShutdownThread getGracefulShutdownThread() {
+    return gracefulShutdownThread;
+  }
+
   public static void main(final String[] cli) throws DrillbitStartupException {
     final StartupOptions options = StartupOptions.parse(cli);
     start(options);
diff --git a/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java b/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java
index 9e3721cf029..9d649ba4521 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/test/TestGracefulShutdown.java
@@ -17,9 +17,11 @@
  */
 package org.apache.drill.test;
 import org.apache.drill.categories.SlowTest;
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.exec.ExecConstants;
 import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
 import org.apache.drill.exec.server.Drillbit;
+import org.apache.hadoop.net.ServerSocketUtil;
 import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Rule;
@@ -36,6 +38,9 @@
 import java.nio.file.Path;
 import java.util.Collection;
 
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 
 @Category({SlowTest.class})
@@ -174,6 +179,44 @@ public void testRestApiShutdown() throws Exception {
     }
   }
 
+  @Test // DRILL-6912
+  public void gracefulShutdownThreadShouldBeInitializedBeforeClosingDrillbit() throws Exception {
+    Drillbit drillbit = null;
+    Drillbit drillbitWithSamePort = null;
+
+    int userPort = ServerSocketUtil.getPort(31170, 300);
+    int bitPort = ServerSocketUtil.getPort(31180, 300);
+    ClusterFixtureBuilder fixtureBuilder = ClusterFixture.bareBuilder(dirTestWatcher).withLocalZk()
+        .configProperty(ExecConstants.INITIAL_USER_PORT, userPort)
+        .configProperty(ExecConstants.INITIAL_BIT_PORT, bitPort);
+    try (ClusterFixture clusterFixture = fixtureBuilder.build()) {
+      drillbit = clusterFixture.drillbit();
+
+      // creating another drillbit instance using same config
+      drillbitWithSamePort = new Drillbit(clusterFixture.config(), fixtureBuilder.configBuilder().getDefinitions(),
+          clusterFixture.serviceSet());
+
+      try {
+        drillbitWithSamePort.run();
+        fail("drillbitWithSamePort.run() should throw UserException");
+      } catch (UserException e) {
+        // it's expected that second drillbit can't be started because port is busy
+        assertThat(e.getMessage(), containsString("RESOURCE ERROR: Drillbit could not bind to port"));
+      }
+    } finally {
+      // preconditions
+      assertNotNull(drillbit);
+      assertNotNull(drillbitWithSamePort);
+      assertNotNull("gracefulShutdownThread should be initialized, otherwise NPE will be thrown from close()",
+          drillbit.getGracefulShutdownThread());
+      // main test case
+      assertNotNull("gracefulShutdownThread should be initialized, otherwise NPE will be thrown from close()",
+          drillbitWithSamePort.getGracefulShutdownThread());
+      drillbit.close();
+      drillbitWithSamePort.close();
+    }
+  }
+
   private static boolean waitAndAssertDrillbitCount(ClusterFixture cluster, int zkRefresh) throws InterruptedException {
 
     while (true) {


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services