Skip to content

GH-8642: Revise executors in the project #8647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jun 14, 2023
Merged

GH-8642: Revise executors in the project #8647

merged 5 commits into from
Jun 14, 2023

Conversation

artembilan
Copy link
Member

Fixes #8642

  • Rework some Executors.newSingleThreadExecutor() to ExecutorServiceAdapter(new SimpleAsyncTaskExecutor())
  • Expose TaskExecutor setters; deprecate ExecutorService-based
  • Some other code clean up in the effected classes: LogAccessor, no synchronized in critical blocks
  • Give a meaningful prefix for default threads in the context of components, e.g. SubscribableRedisChannel - getBeanName() + "-"

@artembilan artembilan added this to the 6.2.0-M1 milestone Jun 13, 2023
@artembilan artembilan requested a review from garyrussell June 13, 2023 16:43
garyrussell
garyrussell suggested changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started looking at this but I stopped after the first class; I don't think using the ExecutorServiceAdapter is correct - why do we need it? Why not use the TE directly?

The default SATE will never be stopped.

@artembilan
Copy link
Member Author

I don't think using the ExecutorServiceAdapter is correct

It is correct when we need a <T> Future<T> submit(Callable<T> task); API and call cancel() for that Future somewhere.
The TE does not provide that hook for us.
Whenever we don't deal with Future I indeed left it as a plain TE and SATE by default.

@garyrussell
Copy link
Contributor

Then force the TE to be an AsyncTaskExecutor instead (that's what I do in SK).

	/**
	 * The executor for threads that poll the consumer.
	 */
	private AsyncTaskExecutor listenerTaskExecutor;

I don't see any way to stop the default TE otherwise.

@artembilan
Copy link
Member Author

Then force the TE to be an AsyncTaskExecutor

Right. Missed that one. Thanks. Let me rework it then!

I don't see any way to stop the default TE otherwise.

Right. With SATE as default we really don't need to worry about its stop, since it does not cache threads and we really don't need them to be cached in those places.
The goal of this PR is to get rid of an ExecutorService as an injection point.

@artembilan
spring-projectsGH-8642: Revise executors in the project
0908bbf 
Fixes spring-projects#8642

* Rework some `Executors.newSingleThreadExecutor()` to `ExecutorServiceAdapter(new SimpleAsyncTaskExecutor())`
* Expose `TaskExecutor` setters; deprecate `ExecutorService`-based
* Some other code clean up in the effected classes: `LogAccessor`, no `synchronized` in critical blocks
* Give a meaningful prefix for default threads in the context of components, e.g. `SubscribableRedisChannel` - `getBeanName() + "-"`
@artembilan
* Fix PostgresChannelMessageTableSubscriberTests for
8d5398d 
`PostgresSubscribableChannel` initialization to let it create
its internal `Executor`
@artembilan
Use an AsyncTaskExecutor injection instead of `ExecutorServiceAdapt…
d2d3698 
…er` wrapping
@artembilan artembilan requested a review from garyrussell June 14, 2023 16:13
@garyrussell
Copy link
Contributor 

org.springframework.beans.NotWritablePropertyException: Invalid property 'executorService' of bean class [org.springframework.integration.support.leader.LockRegistryLeaderInitiator]: Bean property 'executorService' has no matching field.
	at org.springframework.beans.DirectFieldAccessor.createNotWritablePropertyException(DirectFieldAccessor.java:96)
	at org.springframework.beans.AbstractNestablePropertyAccessor.processLocalProperty(AbstractNestablePropertyAccessor.java:431)
	at org.springframework.beans.AbstractNestablePropertyAccessor.setPropertyValue(AbstractNestablePropertyAccessor.java:279)
	at org.springframework.beans.AbstractNestablePropertyAccessor.setPropertyValue(AbstractNestablePropertyAccessor.java:247)
	at org.springframework.integration.support.leader.LockRegistryLeaderInitiatorTests.testGracefulLeaderSelectorExit(LockRegistryLeaderInitiatorTests.java:255)

garyrussell
garyrussell suggested changes Jun 14, 2023
View reviewed changes
...rc/main/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiator.java Outdated
* @deprecated since 6.2 in favor of {@link #setTaskExecutor(AsyncTaskExecutor)}
*/
@Deprecated(since = "6.2", forRemoval = true)
public void setTaskExecutor(ExecutorService taskExecutor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the name change (and param)? Need to deprecate the existing method.

@garyrussell garyrussell merged commit 4db9bad into spring-projects:main Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell approved these changes

Assignees
No one assigned
Labels
in: core type: enhancement
Projects
None yet
Milestone
6.2.0-M1
Development

Successfully merging this pull request may close these issues.

Revise an ExecutorService injections in favor of injected TaskExecutor
2 participants
@artembilan @garyrussell