Allow for configurable timeout of the Scheduler. #92

dpratt · 2020-02-27T19:52:51Z

Modify Scheduler to take responsibility for the lifecycle of
the job ExecutorService only if it's not externally supplied.
Make the shutdown timeout be externally configurable by the user.
Additional logging for when the Scheduler does not shut down cleanly.
Add bits to the POM that allow for building under Java 9+
Have the tests use logback instead of slf4-simple for log output

This addresses #88

- Modify Scheduler to take responsibility for the lifecycle of the job ExecutorService only if it's not externally supplied. - Make the shutdown timeout be externally configurable by the user. - Additional logging for when the Scheduler does not shut down cleanly. - Add bits to the POM that allow for building under Java 9+ - Have the tests use logback instead of slf4-simple for log output

kagkarlsson · 2020-02-27T20:35:59Z

Thanks for the contribution! Will try and have a look in the coming days :)

kagkarlsson · 2020-03-03T05:41:11Z

Bit of a busy week, haven't had the time to check this yet

kagkarlsson

Thanks for contributing to db-scheduler! Looks good, but left a few comments :)

kagkarlsson · 2020-03-09T07:55:30Z

db-scheduler-boot-starter/src/test/resources/logback-test.xml

+<configuration>
+
+  <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
+    <encoder>
+      <pattern>%d{HH:mm:ss.SSS} %highlight(%-5level) %cyan(%logger{50}) - %msg%n%rootException</pattern>
+    </encoder>
+  </appender>
+
+  <logger level="INFO" name="com.github.kagkarlsson"/>
+
+  <root level="WARN">
+    <appender-ref ref="STDOUT"/>
+  </root>
+
+</configuration>


Nice! Was not aware of those options (highlight, cyan), better looking output 👍

kagkarlsson · 2020-03-09T08:11:54Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java

 import java.util.*;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
+import java.util.concurrent.*;


Yeah I see I have a bit of a whitespace issue in the project. I will fix that afterwards, in another PR, to make it more consistent.

(the placement of this comment was probably better before I disabled whitespace changes... )

kagkarlsson · 2020-03-09T08:19:00Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java

+//    this.dueExecutor = Executors.newSingleThreadExecutor(defaultThreadFactoryWithPrefix(THREAD_PREFIX + "-execute-due-"));
+//    this.detectDeadExecutor = Executors.newSingleThreadExecutor(defaultThreadFactoryWithPrefix(THREAD_PREFIX + "-detect-dead-"));
+//    this.updateHeartbeatExecutor = Executors.newSingleThreadExecutor(defaultThreadFactoryWithPrefix(THREAD_PREFIX + "-update-heartbeat-"));
+


To be removed

kagkarlsson · 2020-03-09T08:23:30Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java

+    if(managedExecutorService) {
+			LOG.info("Letting running executions finish. Will wait up to {}.", executorShutdownWait);
+			if (ExecutorUtils.shutdownAndAwaitTermination(executorService, executorShutdownWait)) {
+				LOG.info("Scheduler stopped.");
+			} else {
+        if(!currentlyProcessing.isEmpty()) {
+          String runningJobs = currentlyProcessing.keySet().stream().map(Execution::toString).collect(Collectors.joining("\n"));
+          LOG.warn("Scheduler did not shut down within the timeout. Interrupting the following jobs:\n{}", runningJobs);
+        } else {
+          LOG.warn("Scheduler did not shut down cleanly within the timeout. Terminating the executor.");
+        }
+			  if(!ExecutorUtils.shutdownNowAndAwaitTermination(executorService, Duration.ofSeconds(5))) {
+			    LOG.warn("Scheduler did not terminate within the timeout. The Scheduler pool may be orphaned and have live threads.");
+        }
+        if(!currentlyProcessing.isEmpty()) {
+          String staleJobs = currentlyProcessing.keySet().stream().map(Execution::toString).collect(Collectors.joining("\n"));
+          LOG.warn("Scheduler stopped, but the following jobs were orphaned:\n{}", staleJobs);
+        }
 			}
+		} else {
+    	LOG.info("Not shutting down externally managed job executor.");


Indent is a bit messed up. I will probably do a reformat to 4 spaces after this PR is merged.

kagkarlsson · 2020-03-09T08:41:59Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/SchedulerBuilder.java

        return this;
    }

+    public SchedulerBuilder executorServiceShutdownTimeout(Duration shutdownTimeout) {


Not sure about the name. Especially since it is not applicable for the builder-method executorService where you send in the externally managed one.

What about shutdownTimeout?

kagkarlsson · 2020-03-09T08:45:31Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java

+        new LinkedBlockingQueue<>(),
+        defaultThreadFactoryWithPrefix(prefix)
+    );
+    executor.allowCoreThreadTimeOut(true);


Currently, I don't think that the pool will ever be scaled to less than 3 threads? (Since currently all util-threads run until shutdown with their own sleep)

kagkarlsson · 2020-03-09T08:52:26Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java

+    ThreadPoolExecutor housekeepingExecutor = newThreadPool(THREAD_PREFIX + "-housekeeper-", 3, THREAD_POOL_TIMEOUT);
+    this.dueExecutor = housekeepingExecutor;
+    this.detectDeadExecutor = housekeepingExecutor;
+    this.updateHeartbeatExecutor = housekeepingExecutor;


Not sure about this. The util-threads currently run "forever" (internal sleep). The only difference now will be that the thread-names will be a bit less describing. What do you feel is the benefit here?

Maybe they should be made into a ScheduledExecutorService, but I suspect that generates a bigger change..

kagkarlsson · 2020-03-09T08:55:50Z

db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/ExecutorUtils.java

+	public static ThreadFactory defaultThreadFactoryWithPrefix(String prefix, boolean useDaemonThreads) {
+		return new PrefixingDefaultThreadFactory(prefix, useDaemonThreads);
 	}


I can't seem to find any references to this method (where daemon=true)? Was there meant to be?

kagkarlsson · 2021-03-15T19:31:44Z

Much of these changes have reached master via other PRs. Closing.

kagkarlsson reviewed Mar 9, 2020

View reviewed changes

kagkarlsson added a commit that referenced this pull request May 10, 2020

Added the logback-test configuration from PR #92 by dpratt.

487b654

kagkarlsson mentioned this pull request May 10, 2020

Fix bug in timestamp handling in prepared statement setter for mssql #105

Merged

evenh pushed a commit to evenh/db-scheduler that referenced this pull request Oct 1, 2020

Added the logback-test configuration from PR kagkarlsson#92 by dpratt.

214ba43

kagkarlsson closed this Mar 15, 2021

Uh oh!

Allow for configurable timeout of the Scheduler. #92

Allow for configurable timeout of the Scheduler. #92

Uh oh!

Conversation

dpratt commented Feb 27, 2020

Uh oh!

kagkarlsson commented Feb 27, 2020

Uh oh!

kagkarlsson commented Mar 3, 2020

Uh oh!

kagkarlsson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kagkarlsson commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants