Pipe executor configuration of shouldShutDown to watchdogs #1859
Conversation
…leapis#1858) BREAKING CHANGE: change WatchdogProvider interface to require whether or not to shut the executor down
johnjcasey
force-pushed
the
bugfix/1858-push-shouldShutDown-to-watchdogs
branch
from
d08c737
to
8a6daac
Compare
| if (shouldShutdownExecutor) { | ||
| return executor.isShutdown(); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Should this return whether the future is done instead of returning true in the case where we aren't shutting down the executor?
| if (shouldShutdownExecutor){ | ||
| return executor.isTerminated(); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Should this return whether the future is done instead of returning true in the case where we aren't shutting down the executor?
| if (shouldShutdownExecutor) { | ||
| return executor.awaitTermination(duration, unit); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Should this wait for the future to complete with the specified time limit instead of returning true?
| @@ -70,20 +70,22 @@ public final class Watchdog implements Runnable, BackgroundResource { | |||
| private final ApiClock clock; | |||
| private final Duration scheduleInterval; | |||
| private final ScheduledExecutorService executor; | |||
| private final boolean shouldShutdownExecutor; | |||
| private ScheduledFuture<?> future; | |||
|
|
|||
| /** returns a Watchdog which is scheduled at the provided interval. */ | |||
| public static Watchdog create( | |||
There was a problem hiding this comment.
This is changing a public method that is not marked as InternalApi which is strictly forbidden.
| @@ -45,7 +45,7 @@ public interface WatchdogProvider { | |||
|
|
|||
| boolean needsExecutor(); | |||
|
|
|||
| WatchdogProvider withExecutor(ScheduledExecutorService executor); | |||
| WatchdogProvider withExecutor(ScheduledExecutorService executor, boolean shouldShutdownExecutor); | |||
There was a problem hiding this comment.
Changing interface is also strictly forbidden.
| @@ -138,12 +140,18 @@ public void shutdown() { | |||
|
|
|||
| @Override | |||
| public boolean isShutdown() { | |||
| return executor.isShutdown(); | |||
| if (shouldShutdownExecutor) { | |||
There was a problem hiding this comment.
What exactly is the problem we are trying to solve here? Is the problem that Watchdog shutting down executor regardless of the shouldAutoClose() flag or Watchdog reporting isShutdown() regardless of the shouldAutoClose() flag? The implementation seems try to solve the latter.
|
the issue is that the watchdog is trying to shut down the executor regardless of the executors shouldAutoClose flag |
This fixes the issue identified in #1858