-
Notifications
You must be signed in to change notification settings - Fork 581
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 2 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
124 changes: 124 additions & 0 deletions
124
...n/java/org/springframework/cloud/dataflow/server/config/AsyncConfigurationProperties.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,124 @@ | ||
/* | ||
* 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 org.springframework.boot.context.properties.ConfigurationProperties; | ||
import org.springframework.cloud.dataflow.core.DataFlowPropertyKeys; | ||
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; | ||
import org.springframework.scheduling.concurrent.ExecutorConfigurationSupport; | ||
|
||
/** | ||
* Used to configure the {@link ThreadPoolTaskExecutor} in {@link DataflowAsyncConfiguration}. For more information | ||
* of the fields see {@link ThreadPoolTaskExecutor} and {@link ExecutorConfigurationSupport} | ||
* | ||
* @author Tobias Soloschenko | ||
*/ | ||
@ConfigurationProperties(prefix = AsyncConfigurationProperties.ASYNC_PREFIX) | ||
public class AsyncConfigurationProperties { | ||
|
||
public static final String ASYNC_PREFIX = DataFlowPropertyKeys.PREFIX + "async"; | ||
|
||
private int corePoolSize = 1; | ||
|
||
private int maxPoolSize = Integer.MAX_VALUE; | ||
|
||
private int keepAliveSeconds = 60; | ||
|
||
private int queueCapacity = Integer.MAX_VALUE; | ||
|
||
private boolean allowCoreThreadTimeOut = false; | ||
|
||
private boolean prestartAllCoreThreads = false; | ||
|
||
private boolean waitForTasksToCompleteOnShutdown = false; | ||
|
||
private long awaitTerminationMillis = 0L; | ||
|
||
private String threadNamePrefix = "scdf-async-"; | ||
|
||
public int getQueueCapacity() { | ||
return queueCapacity; | ||
} | ||
|
||
public void setCorePoolSize(int corePoolSize) { | ||
this.corePoolSize = corePoolSize; | ||
} | ||
|
||
public int getMaxPoolSize() { | ||
return maxPoolSize; | ||
} | ||
|
||
public void setMaxPoolSize(int maxPoolSize) { | ||
this.maxPoolSize = maxPoolSize; | ||
} | ||
|
||
public int getKeepAliveSeconds() { | ||
return keepAliveSeconds; | ||
} | ||
|
||
public void setKeepAliveSeconds(int keepAliveSeconds) { | ||
this.keepAliveSeconds = keepAliveSeconds; | ||
} | ||
|
||
public void setQueueCapacity(int queueCapacity) { | ||
this.queueCapacity = queueCapacity; | ||
} | ||
|
||
public int getCorePoolSize() { | ||
return corePoolSize; | ||
} | ||
|
||
public boolean isAllowCoreThreadTimeOut() { | ||
return allowCoreThreadTimeOut; | ||
} | ||
|
||
public void setAllowCoreThreadTimeOut(boolean allowCoreThreadTimeOut) { | ||
this.allowCoreThreadTimeOut = allowCoreThreadTimeOut; | ||
} | ||
|
||
public boolean isPrestartAllCoreThreads() { | ||
return prestartAllCoreThreads; | ||
} | ||
|
||
public void setPrestartAllCoreThreads(boolean prestartAllCoreThreads) { | ||
this.prestartAllCoreThreads = prestartAllCoreThreads; | ||
} | ||
|
||
public boolean isWaitForTasksToCompleteOnShutdown() { | ||
return waitForTasksToCompleteOnShutdown; | ||
} | ||
|
||
public void setWaitForTasksToCompleteOnShutdown(boolean waitForTasksToCompleteOnShutdown) { | ||
this.waitForTasksToCompleteOnShutdown = waitForTasksToCompleteOnShutdown; | ||
} | ||
|
||
public long getAwaitTerminationMillis() { | ||
return awaitTerminationMillis; | ||
} | ||
|
||
public void setAwaitTerminationMillis(long awaitTerminationMillis) { | ||
this.awaitTerminationMillis = awaitTerminationMillis; | ||
} | ||
|
||
public String getThreadNamePrefix() { | ||
return threadNamePrefix; | ||
} | ||
|
||
public void setThreadNamePrefix(String threadNamePrefix) { | ||
this.threadNamePrefix = threadNamePrefix; | ||
} | ||
} |
58 changes: 58 additions & 0 deletions
58
...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,58 @@ | ||
/* | ||
* 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.springframework.boot.context.properties.EnableConfigurationProperties; | ||
import org.springframework.context.annotation.Configuration; | ||
import org.springframework.scheduling.annotation.AsyncConfigurer; | ||
import org.springframework.scheduling.annotation.EnableAsync; | ||
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; | ||
|
||
/** | ||
* 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) | ||
@EnableAsync | ||
@EnableConfigurationProperties(AsyncConfigurationProperties.class) | ||
public class DataflowAsyncConfiguration implements AsyncConfigurer { | ||
|
||
private final AsyncConfigurationProperties asyncConfigurationProperties; | ||
|
||
public DataflowAsyncConfiguration(AsyncConfigurationProperties asyncConfigurationProperties) { | ||
this.asyncConfigurationProperties = asyncConfigurationProperties; | ||
} | ||
|
||
@Override | ||
public Executor getAsyncExecutor() { | ||
ThreadPoolTaskExecutor threadPoolTaskExecutor = new ThreadPoolTaskExecutor(); | ||
threadPoolTaskExecutor.setQueueCapacity(asyncConfigurationProperties.getQueueCapacity()); | ||
threadPoolTaskExecutor.setCorePoolSize(asyncConfigurationProperties.getCorePoolSize()); | ||
threadPoolTaskExecutor.setMaxPoolSize(asyncConfigurationProperties.getMaxPoolSize()); | ||
threadPoolTaskExecutor.setKeepAliveSeconds(asyncConfigurationProperties.getKeepAliveSeconds()); | ||
threadPoolTaskExecutor.setAllowCoreThreadTimeOut(asyncConfigurationProperties.isAllowCoreThreadTimeOut()); | ||
threadPoolTaskExecutor.setPrestartAllCoreThreads(asyncConfigurationProperties.isPrestartAllCoreThreads()); | ||
threadPoolTaskExecutor.setAwaitTerminationMillis(asyncConfigurationProperties.getAwaitTerminationMillis()); | ||
threadPoolTaskExecutor.setThreadNamePrefix(asyncConfigurationProperties.getThreadNamePrefix()); | ||
threadPoolTaskExecutor.setWaitForTasksToCompleteOnShutdown(asyncConfigurationProperties.isWaitForTasksToCompleteOnShutdown()); | ||
threadPoolTaskExecutor.initialize(); | ||
return threadPoolTaskExecutor; | ||
} | ||
} |
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.