feat: async task execution for cleanup - fix for #5408 #5412
Conversation
klopfdreh
changed the title
|
If there are further operations that require a long time to be processed, just use |
|
@onobc - I hope it is ok to ask you directly as you helped me also with the integration of: #5408. With this implementation the client does not rely on the finished response from server when all tasks / executions are cleaned up, but receives a direct response from the server. The server is cleaning up all tasks / executions in the background then. |
|
Not a problem w/ the direct ping @klopfdreh . We are very busy making sure we hit the 2.11.0 updated date but I will try to get this reviewed and hopefully in for the release. If you don't see any activity on this in the next 48 hrs, please ping me here again. Thanks |
|
As requested @onobc - ping! 😁 |
There was a problem hiding this comment.
@klopfdreh this looks good. I have some requested changes (mainly around simplifying). Also, we need to add a test (or two) for whatever we add here.
Thanks for the contribution. We are tight on time but I am able to give this another review in the next 24hrs.
| @@ -265,6 +266,7 @@ public void cleanup(@PathVariable("id") Set<Long> ids, | |||
| */ | |||
| @RequestMapping(method = RequestMethod.DELETE) | |||
| @ResponseStatus(HttpStatus.OK) | |||
| @Async | |||
There was a problem hiding this comment.
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).
- ❌ UI will not pick this up and you must use this via REST API
- ➕ low risk of breaking other users
- ➕ UI does not give the false appearance that all has been cleaned up instantly
- ➕ can add CompletableFuture return value for better usage later from UI/callers etc..
Option 2
Guard the auto-config w/ enabled property - hence w/o @EnableAsync the @Async is invisible.
- ❌ UI will give false appearance that all has been cleaned up instantly when async is enabled
- ❌ moderate risk of breaking/confusing other users
- ➕ for those users that opt-in, the UI does this async
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.
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.
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.
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.
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.
Just one little question regarding Option 2. The current refactoring includes the ConditionalOnProperty 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.
spring-cloud-dataflow-server-core/src/main/resources/META-INF/spring.factories
Show resolved
Hide resolved
|
The async processing might be used also for other endpoints in addition to task cleanup. So I changed it to a more generic prefix |
|
Closed via 79845a0 |
Fixes: #5408
To be able to clean up a lot of task executions the servlet requests needs to be committed. If the timeout on client side of the http request is too small this might cause an issue and the operation is rolled back, even if the task executions could be deleted.
This PR introduces Spring Async to Rest-Controllers so that the request is committed immediately and the server processes the request in the background.
Also you can disable the
DataflowAsyncConfigurationto process the Request/Response as before.As you can see here we have some task executions:
Now we are going to delete them:
With
DataflowAsyncConfigurationenabled you can see that the deletion is handled in theThreadPoolTaskExecutor:Without the
DataflowAsyncConfigurationthe requests/responses are processed as before: