-
Notifications
You must be signed in to change notification settings - Fork 119
Pipe executor configuration of shouldShutDown to watchdogs #1859
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
d08c737
to
8a6daac
Compare
if (shouldShutdownExecutor) { | ||
return executor.isShutdown(); | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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