[GitHub] [drill] sohami commented on a change in pull request #1848: DRILL-7360: Refactor WatchService in Drillbit class and fix concurrency issues

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

sohami commented on a change in pull request #1848: DRILL-7360: Refactor WatchService in Drillbit class and fix concurrency issues
URL: https://github.com/apache/drill/pull/1848#discussion_r317244970
 
 

 ##########
 File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java
 ##########
 @@ -371,65 +376,91 @@ private void javaPropertiesToSystemOptions() {
     }
   }
 
-
-  // Polls for graceful file to check if graceful shutdown is triggered from the script.
+  /**
+   * Polls for graceful file to check if graceful shutdown is triggered from the script.
+   */
   private static class GracefulShutdownThread extends Thread {
 
+    private static final String DRILL_HOME = "DRILL_HOME";
+    private static final String GRACEFUL_SIGFILE = "GRACEFUL_SIGFILE";
+    private static final String NOT_SUPPORTED_MESSAGE = "Graceful shutdown from command line will not be supported.";
+
     private final Drillbit drillbit;
     private final StackTrace stackTrace;
-    GracefulShutdownThread(final Drillbit drillbit, final StackTrace stackTrace) {
+
+    GracefulShutdownThread(Drillbit drillbit, StackTrace stackTrace) {
       this.drillbit = drillbit;
       this.stackTrace = stackTrace;
+
+      setName("Drillbit-Graceful-Shutdown#" + getName());
     }
 
     @Override
     public void run () {
       try {
         pollShutdown(drillbit);
-      } catch (InterruptedException  e) {
-        logger.debug("Interrupted GracefulShutdownThread");
+      } catch (InterruptedException e) {
+        drillbit.interruptPollShutdown = false;
+        logger.debug("Graceful Shutdown thread was interrupted", e);
       } catch (IOException e) {
-        throw new RuntimeException("Caught exception while polling for gracefulshutdown\n" + stackTrace, e);
+        throw new RuntimeException("Exception while polling for graceful shutdown\n" + stackTrace, e);
       }
     }
 
-    /*
-     * Poll for the graceful file, if the file is found cloase the drillbit. In case if the DRILL_HOME path is not
-     * set, graceful shutdown will not be supported from the command line.
+    /**
+     * Poll for the graceful file, if the file is found or modified, close the Drillbit.
+     * In case if the {@link #DRILL_HOME} or {@link #GRACEFUL_SIGFILE} environment variables are not set,
+     * graceful shutdown will not be supported from the command line.
+     *
+     * @param drillbit current Drillbit
      */
     private void pollShutdown(Drillbit drillbit) throws IOException, InterruptedException {
-      final String drillHome = System.getenv("DRILL_HOME");
-      final String gracefulFile = System.getenv("GRACEFUL_SIGFILE");
-      final Path drillHomePath;
+      String drillHome = System.getenv(DRILL_HOME);
+      String gracefulFile = System.getenv(GRACEFUL_SIGFILE);
+      Path drillHomePath;
       if (drillHome == null || gracefulFile == null) {
-        logger.warn("Cannot access graceful file. Graceful shutdown from command line will not be supported.");
+        if (logger.isWarnEnabled()) {
+          StringBuilder builder = new StringBuilder(NOT_SUPPORTED_MESSAGE);
+          if (drillHome == null) {
+            builder.append(" ").append(DRILL_HOME).append(" is not set.");
+          }
+          if (gracefulFile == null) {
+            builder.append(" ").append(GRACEFUL_SIGFILE).append(" is not set.");
+          }
+          logger.warn(builder.toString());
+        }
         return;
       }
       try {
         drillHomePath = Paths.get(drillHome);
+        if (!Files.exists(drillHomePath)) {
+          logger.warn("{} path [{}] does not exist. {}", DRILL_HOME, drillHomePath, NOT_SUPPORTED_MESSAGE);
+          return;
+        }
       } catch (InvalidPathException e) {
-        logger.warn("Cannot access graceful file. Graceful shutdown from command line will not be supported.");
+        logger.warn("Unable to construct {}} path [{}]: {}. {}",
+          DRILL_HOME, drillHome, e.getMessage(), NOT_SUPPORTED_MESSAGE);
         return;
       }
-      boolean triggered_shutdown = false;
-      WatchKey wk = null;
-      try (final WatchService watchService = drillHomePath.getFileSystem().newWatchService()) {
-        drillHomePath.register(watchService, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.ENTRY_CREATE);
-        while (!triggered_shutdown) {
-          wk = watchService.take();
-          for (WatchEvent<?> event : wk.pollEvents()) {
-            final Path changed = (Path) event.context();
-            if (changed != null && changed.endsWith(gracefulFile)) {
-              drillbit.interruptPollShutdown = false;
-              triggered_shutdown = true;
-              drillbit.close();
-              break;
+
+      try (WatchService watchService = drillHomePath.getFileSystem().newWatchService()) {
+        drillHomePath.register(watchService, StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_MODIFY);
+        while (true) {
+          WatchKey watchKey = watchService.take();
+          for (WatchEvent<?> event : watchKey.pollEvents()) {
+            if (StandardWatchEventKinds.OVERFLOW != event.kind()) {
+              Path changedPath = (Path) event.context();
+              if (changedPath != null && changedPath.endsWith(gracefulFile)) {
+                drillbit.interruptPollShutdown = false;
+                drillbit.close();
+                return;
+              }
             }
           }
-        }
-      } finally {
-        if (wk != null) {
-          wk.cancel();
+          if (!watchKey.reset()) {
+            drillbit.interruptPollShutdown = false;
+            return;
+          }
 
 Review comment:
   Thanks for adding the call to reset. But I think there is still some more changes needed here. If `watchkey.reset()` returns false, in our case that means we have not seen any event related to graceful file and still observing some failure while resetting the watchKey. In this case we should try to register again because if we returned from here that means GracefulShutdown thread will exit with Drillbit still up and running. Any future attempt for graceful shutdown will not work.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services