Add support for setting the awaitTeminationSeconds and waitForTasksToCompleteOnShutdown #15951

filiphr · 2019-02-14T10:42:07Z

to the TaskExecutorBuilder and via the TaskExecutionProperties

I started the migration to Spring Boot 2.1 and I wanted to get rid of our custom executor configuration as we can reuse the one from Boot. Awesome job on adding that in 2.1. I noticed that there are 2 methods that we use and are not exposed. With this PR I am exposing those 2 properties.

Let me know if the target to 2.1.x and the PR in general is acceptable for you. If there is something that I need to change please do let me know.

…CompleteOnShutdown to the TaskExecutorBuilder and via the TaskExecutionProperties

snicoll

Thanks for the PR, this looks good overall and I am not sure I remember why I didn't added those. I've added a few suggestions and we'll figure out next where that should land. In theory, master is the rule.

snicoll · 2019-02-14T10:53:06Z

...igure/src/main/java/org/springframework/boot/autoconfigure/task/TaskExecutionProperties.java

@@ -36,6 +39,22 @@
 	 */
 	private String threadNamePrefix = "task-";

+	/**
+	 * The maximum number of time that the executor is supposed to block on shutdown in


Configuration properties descriptions do not start with "The", "A", etc. The first sentence must be much shorter as it's used by IDEs on auto-completion.

Wasn't aware about that. Will change it to

The maximum number of time that the executor is supposed to block on shutdown waiting for remaining tasks to complete.

Is this text acceptable?

No it is isn't. It starts with The and I've indicated previously we're not documenting keys that way.

On a second thought those two keys are related to shutdown so it might be interesting to crete a sub group for them the same way we did for pool

Sorry, I actually removed The in my commit. Yes I'll move them into a shutdown sub group

snicoll · 2019-02-14T10:53:22Z

...igure/src/main/java/org/springframework/boot/autoconfigure/task/TaskExecutionProperties.java

+	private Duration awaitTermination;
+
+	/**
+	 * Whether the executor should wait for scheduled tasks to complete on shutdown, not


Ditto. The sentence should stop at the current comma IMO.

snicoll · 2019-02-14T10:55:52Z

...st/java/org/springframework/boot/autoconfigure/task/TaskExecutionAutoConfigurationTests.java

+				"spring.task.execution.pool.allow-core-thread-timeout=true",
+				"spring.task.execution.pool.keep-alive=5s",
+				"spring.task.execution.await-termination=30s",
+				"spring.task.execution.wait-for-tasks-to-complete-on-shutdown=true",


Let's try to keep a consistent ordering in things. Adding those two properties at the end looks ok to me as they are logically grouped. Let's keep the same here and add them after the thread-name-prefix

I initially wanted not to create a lot of unnecessary changes, but the format already did that 😄. I will change the ordering

snicoll · 2019-02-14T10:56:43Z

...st/java/org/springframework/boot/autoconfigure/task/TaskExecutionAutoConfigurationTests.java

@@ -79,6 +81,10 @@ public void taskExecutorBuilderShouldApplyCustomSettings() {
 							.hasFieldOrPropertyWithValue("allowCoreThreadTimeOut", true);
 					assertThat(taskExecutor.getKeepAliveSeconds()).isEqualTo(5);
 					assertThat(taskExecutor.getThreadNamePrefix()).isEqualTo("mytest-");
+					assertThat(ReflectionTestUtils.getField(taskExecutor,


We don't use ReflectionTestUtils for such basic assertion but assertJ (assertFieldOrPropertyWithValue).

snicoll · 2019-02-14T10:57:14Z

spring-boot-project/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc

@@ -175,6 +175,8 @@ content into your application. Rather, pick only the properties that you need.
 	spring.task.execution.pool.max-size= # Maximum allowed number of threads. If tasks are filling up the queue, the pool can expand up to that size to accommodate the load. Ignored if the queue is unbounded.
 	spring.task.execution.pool.queue-capacity= # Queue capacity. An unbounded capacity does not increase the pool and therefore ignores the "max-size" property.
 	spring.task.execution.thread-name-prefix=task- # Prefix to use for the names of newly created threads.
+	spring.task.execution.await-termination= # The maximum number of time that the executor is supposed to block on shutdown in order to wait for remaining tasks to complete their execution before the rest of the container continues to shut down. This is particularly useful if your remaining tasks are likely to need access to other resources that are also managed by the container. If a duration suffix is not specified, seconds will be used.


Keys are ordered alphabetically and the description is way too long as well.

snicoll · 2019-02-14T10:57:45Z

...oot-project/spring-boot/src/main/java/org/springframework/boot/task/TaskExecutorBuilder.java

+	}
+
+	/**
+	 * /** Set the maximum number of time that the executor is supposed to block on


Formatting glitch here.

filiphr · 2019-02-14T11:46:39Z

I've added a few suggestions and we'll figure out next where that should land. In theory, master is the rule.

Thanks for the review @snicoll. I've applied the fixes you mentioned. If the text is still not satisfiable I can adapt it. I more or less copied that from the ExecutorConfigurationSupport.

The reason why I did it against 2.1.x was that I was hoping it might go in it so I can remove more from our code. I also saw that there were a lot of commits on 2.1.x and which were then merged to master.

Feel free to squash everything in one commit, if you want me to do that let me know

snicoll · 2019-02-14T14:12:10Z

@filiphr thanks but I believe the workaround for now is easy enough. You can inject the builder, apply it and then set the two properties you need. It doesn't remove your custom arrangement but it allows to configure the 2.1 properties at least.

I need to give a bit this a bit more thought. As I've indicated, those properties are related to shutdown so I am quite keen to group them, especially as the current form is too long IMO.

I'll pick up from here.

filiphr · 2019-02-14T15:14:15Z

Yes the current workaround works perfectly.

I need to give a bit this a bit more thought. As I've indicated, those properties are related to shutdown so I am quite keen to group them, especially as the current form is too long IMO.

I'll pick up from here.

No problem, if there is something that I can do please let me know.

See gh-15951

* pr/15951: Add support for task scheduling shutdown related properties Polish "Add support for task executor shutdown related properties" Add support for task executor shutdown related properties

snicoll · 2019-02-18T20:15:19Z

@filiphr thanks for the contribution. I've polished it and also added support for TaskScheduling as they share a common feature there.

filiphr · 2019-02-18T20:21:20Z

Thanks for merging the PR and for extending on it

Add support for setting the awaitTeminationSeconds and waitForTasksTo…

51cd837

…CompleteOnShutdown to the TaskExecutorBuilder and via the TaskExecutionProperties

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 14, 2019

snicoll reviewed Feb 14, 2019

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Feb 14, 2019

Apply review comments

8339e6d

snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 15, 2019

snicoll added this to the 2.2.x milestone Feb 15, 2019

snicoll changed the base branch from 2.1.x to master February 18, 2019 16:53

snicoll self-assigned this Feb 18, 2019

snicoll modified the milestones: 2.2.x, 2.2.0.M1 Feb 18, 2019

snicoll pushed a commit that referenced this pull request Feb 18, 2019

Add support for task executor shutdown related properties

3b47ba2

See gh-15951

snicoll closed this in d2cbf08 Feb 18, 2019

snicoll added a commit that referenced this pull request Feb 18, 2019

Add support for task scheduling shutdown related properties

fa49dfc

See gh-15951

filiphr deleted the task-execution-configuration-more-properties branch February 18, 2019 20:19

savinov mentioned this pull request Sep 13, 2022

waitForTasksToCompleteOnShutdown has no effect on tasks created with @Scheduled annotation spring-projects/spring-framework#26482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for setting the awaitTeminationSeconds and waitForTasksToCompleteOnShutdown #15951

Add support for setting the awaitTeminationSeconds and waitForTasksToCompleteOnShutdown #15951

filiphr commented Feb 14, 2019

snicoll left a comment

snicoll Feb 14, 2019

filiphr Feb 14, 2019

snicoll Feb 14, 2019

filiphr Feb 14, 2019

snicoll Feb 14, 2019

snicoll Feb 14, 2019

filiphr Feb 14, 2019

snicoll Feb 14, 2019

snicoll Feb 14, 2019

snicoll Feb 14, 2019

filiphr commented Feb 14, 2019

snicoll commented Feb 14, 2019

filiphr commented Feb 14, 2019

snicoll commented Feb 18, 2019

filiphr commented Feb 18, 2019

Add support for setting the awaitTeminationSeconds and waitForTasksToCompleteOnShutdown #15951

Add support for setting the awaitTeminationSeconds and waitForTasksToCompleteOnShutdown #15951

Conversation

filiphr commented Feb 14, 2019

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filiphr commented Feb 14, 2019

snicoll commented Feb 14, 2019

filiphr commented Feb 14, 2019

snicoll commented Feb 18, 2019

filiphr commented Feb 18, 2019