-
Notifications
You must be signed in to change notification settings - Fork 580
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.
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
feat: async task execution for cleanup - fix for #5408 #5412
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b4b1213
feat: async task execution for cleanup
klopfdreh d725790
fix: class headers
klopfdreh 36ad9d5
fix: async cleanup review adjustments
klopfdreh e3c9052
fix: more generic name for executor
klopfdreh 8558609
fix: more generic prefix for properties
klopfdreh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
62 changes: 62 additions & 0 deletions
62
...ain/java/org/springframework/cloud/dataflow/server/config/DataflowAsyncConfiguration.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* | ||
* Copyright 2016-2023 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.cloud.dataflow.server.config; | ||
|
||
import java.util.concurrent.Executor; | ||
|
||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.aop.interceptor.AsyncUncaughtExceptionHandler; | ||
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; | ||
import org.springframework.boot.context.properties.EnableConfigurationProperties; | ||
import org.springframework.boot.task.TaskExecutorBuilder; | ||
import org.springframework.cloud.dataflow.core.DataFlowPropertyKeys; | ||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.context.annotation.Configuration; | ||
import org.springframework.scheduling.annotation.AsyncConfigurer; | ||
import org.springframework.scheduling.annotation.EnableAsync; | ||
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; | ||
|
||
import static org.springframework.cloud.dataflow.server.config.DataflowAsyncConfiguration.ASYNC_PREFIX; | ||
|
||
/** | ||
* Class to override the executor at the application level. It also enables async executions for the Spring Cloud Data Flow Server. | ||
* | ||
* @author Tobias Soloschenko | ||
*/ | ||
@Configuration(proxyBeanMethods = false) | ||
@ConditionalOnProperty(prefix = ASYNC_PREFIX, name = "enabled") | ||
@EnableAsync | ||
class DataflowAsyncConfiguration implements AsyncConfigurer { | ||
|
||
private static final Logger logger = LoggerFactory.getLogger(DataflowAsyncConfiguration.class); | ||
|
||
public static final String ASYNC_PREFIX = DataFlowPropertyKeys.PREFIX + "async"; | ||
|
||
private static final String THREAD_NAME_PREFIX = "scdf-async-"; | ||
|
||
@Bean(name = "asyncExecutor") | ||
Executor getAsyncExecutor(TaskExecutorBuilder taskExecutorBuilder) { | ||
return taskExecutorBuilder.threadNamePrefix(THREAD_NAME_PREFIX).build(); | ||
} | ||
|
||
@Override | ||
public AsyncUncaughtExceptionHandler getAsyncUncaughtExceptionHandler() { | ||
return (throwable, method, objects) -> logger.error("Exception thrown in @Async Method " + method.getName(), | ||
throwable); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 needs to be an opt-in feature and here are a couple of options I can think of:
Option 1
Add another API (eg.
cleanupAllAsync
).Option 2
Guard the auto-config w/
enabled
property - hence w/o@EnableAsync
the@Async
is invisible.I am in favor of the latter option. If you agree, please prototype/verify the absence of
@EnableAsync
does what I think it does.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.
I don’t know why it should be an opt in feature as the UI says: „X task executions will be deleted“ - it doesn’t matter if the user can interact directly after pressing ok (in case of async) or has to wait till the backend processed the operation. (in case of sync)
In case this async option is used somewhere else you are right the api has to be designed to represent what is going on like „…will be…“
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.
I hear you. That is a good point. My concern is
w/o some sort of messaging in the UI that mentions that it will happen in the background I fear some users may start looking around and still see the executions and then try again, then run into an issue, then file bug report, etc..
whether opt-in or opt-out, we need some sort of "light switch" for the feature in case we run into unforeseen issues in production.
Because it is late in the release cycle I am just being extra cautious. I do want to get this feature in, but I also am treading carefully.
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.
Good point!
As you prefer we can delay this feature as with „clean task executions after n days“ you can run multiple cleanups and shrink down the count of present executions in multiple steps, so there is no need to hurry up with this anymore.😃
I am going to refactor and add the simplifications and we can think about a good way to implement this in a following release.
WDYT?
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.
That would be great and I would appreciate that. I know you put your hard work and effort into this so I wanted to match you w/ whatever effort I could to get this in. Moving it to post 2.11 is the smart decision. Thank you.
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.
Just one little question regarding
Option 2
. The current refactoring includes theConditionalOnProperty
to check if the functionality is enabled so you have to opt in to use this feature.From my understanding
@EnableAsync
has to be used with@Async
as the annotation on the method picks up the thread pool executor provided with@EnableAsync
- but I am not 100% sure.Here are some information regarding async implementation: https://spring.io/guides/gs/async-method/ and https://stackoverflow.com/a/53357076
If this is the case I would suggest to still let
@EnableAsync
be on the Configuration, because it is not used when the opt flag in is not set.Edit: I switched back the name of the bean and the prefix as this might be used for other async methods as well - not only for cleanup.