feat: task cleanup before end time in days by klopfdreh · Pull Request #5422 · spring-attic/spring-cloud-dataflow

klopfdreh · 2023-08-02T08:56:39Z

Improvement:

Cleanup task executions older than x days.

Fix:

Cleanup in management is working now, after the boot 3 changes.

Fixes: #5423 / #5442
Needs to be integrated with: spring-attic/spring-cloud-dataflow-ui#1940

onobc · 2023-08-29T16:59:57Z

@klopfdreh thanks again for another contribution! Do you mind rebasing and merging as there are currently conflicts and it is a bit difficult to see what has changed in main vs. in this PR (I did take a pass at that but realized it may be better if you could do that). I will keep an eye out and re-review once that is done.

Thanks.

klopfdreh · 2023-08-29T17:02:04Z

@onobc - okidoki - I am going to do so, tomorrow! 👍

klopfdreh · 2023-08-29T17:26:09Z

@onobc - well tomorrow in 23 mins 😆

Edit: I also merged: spring-attic/spring-cloud-dataflow-ui#1940 but @oodamien already mentioned that the UI needs to be reworked a bit.

onobc · 2023-08-30T04:32:29Z

Hi @klopfdreh ,
I just now got back around to look at the updates and I realized you did a merge. This still leaves the changes unclear once I pull them down to review. When I said "Do you mind rebasing and merging" I should have said "Do you mind rebasing/squashing and handling the 2 conflicting files". Ideally there will be a single commit over the latest changes in main so that I can easily tell what you changed.

Sorry for the back and forth. I will make this my top priority tomorrow am (or whenever you get the updates in). Thanks.

klopfdreh · 2023-08-30T08:44:41Z

Ah ok. I created a single commit with all changes and force pushed it to this PR. Just to ensure to have a backup: https://github.com/klopfdreh/spring-cloud-dataflow/tree/feature/cleanupEndTimeBackup

onobc

@klopfdreh thanks for the rebase+squash - it was super helpful. I have left a handful of comments to address.

Also, I noticed you added no tests. Then I quickly realized you added no tests because there are no tests in this newly added module. Since they are not in place now I will not trouble w/ you that burden - we will create the tests and include these new APIs in those tests.

...re/src/main/java/org/springframework/cloud/dataflow/server/service/TaskExecutionService.java

...-core/src/main/java/org/springframework/cloud/dataflow/server/service/TaskDeleteService.java

...ringframework/cloud/dataflow/aggregate/task/impl/AggregateDataFlowTaskExecutionQueryDao.java

...in/java/org/springframework/cloud/dataflow/server/service/impl/DefaultTaskDeleteService.java

...java/org/springframework/cloud/dataflow/server/service/impl/DefaultTaskExecutionService.java

...k/src/main/java/org/springframework/cloud/dataflow/aggregate/task/AggregateTaskExplorer.java

onobc · 2023-08-31T04:52:25Z

Thanks for the updates @klopfdreh . I replied to one of your comments but it is late (here) and I will continue the review in the morning.

klopfdreh · 2023-08-31T05:15:28Z

@klopfdreh thanks for the rebase+squash - it was super helpful. I have left a handful of comments to address.

Also, I noticed you added no tests. Then I quickly realized you added no tests because there are no tests in this newly added module. Since they are not in place now I will not trouble w/ you that burden - we will create the tests and include these new APIs in those tests.

Thanks a lot for helping me out and create tests. I have only a limited time to file in changes.

...ringframework/cloud/dataflow/aggregate/task/impl/AggregateDataFlowTaskExecutionQueryDao.java

onobc · 2023-09-01T14:00:27Z

...java/org/springframework/cloud/dataflow/server/service/impl/DefaultTaskExecutionService.java

+
+	@Override
+	public Integer getAllTaskExecutionsCount(boolean onlyCompleted, String taskName, Integer days) {
+		if (days != null) {


@klopfdreh thanks for the updates to the cleanup path - looks great. I want to get this merged but I think the "what to do w/ END_TIME null when user specifies onlyCompleted='false'" still applies to the executions count path as well.

I totally understand you have limited time - thank you for. your patience while we go through our back/forth :) .

Let's figure out what we want to do here and I am more than willing to make those changes and we can co-author this - unless you have the cycles to make another update.

Case 1

If includeTasksEndedMinDaysAgo is specified then that implies onlyCompleted=true - it has to be in order to compare the dates.

the new API getAllTaskExecutionsCount(boolean onlyCompleted, String taskName, Integer includeTasksEndedMinDaysAgo) can have its onlyCompleted param removed and it would solve this case

Case 2

If includeTasksEndedMinDaysAgo is NOT specified then onlyCompleted=true|false makes sense.

the already existing API getAllTaskExecutionsCount(boolean onlyCompleted, String taskName) solves this one

Wdyt?

You are completely right with the scenario if the END_TIME is null the onlyCompleted=false should still apply for counts! Thanks your a lot for the time to review and for the hints!

Sadly I am busy by solving issues at our integration and I don’t know when I am able to revisit to make changes again.

I would appreciate it very much if you could assist me a bit with those changes.

As this feature very important - I guess also for a lot of users of SCDF - it would be very nice to see it integrated soon as your time allows it.

Thank you a lot for the help and the code reviews and of course for the assistance!

Thanks your a lot for the time to review and for the hints!

You are more than welcome - anytime.

Sadly I am busy by solving issues at our integration and I don’t know when I am able to revisit to make changes again.
I would appreciate it very much if you could assist me a bit with those changes.

No worries @klopfdreh . I will get it updated and integrated this weekend. ☮️ and thanks again.

See spring-attic#5422

onobc · 2023-09-04T06:54:21Z

Verifying my commits on top of @klopfdreh in #5453.

If that passes the CI PR then we will merge it and close this one out a super ceded by it.

* Add num days to task cleanup. See #5422 Co-authored-by: Tobias Soloschenko <tsoloschenko@apache.org>

onobc · 2023-09-04T17:51:44Z

Superceded by. #5453

klopfdreh mentioned this pull request Aug 2, 2023

feat: cleanup task executions older than x days spring-attic/spring-cloud-dataflow-ui#1940

Merged

klopfdreh mentioned this pull request Aug 29, 2023

Add num days option to task cleanup #5442

Closed

onobc added this to the 2.11.0 milestone Aug 29, 2023

feat: task cleanup before end time in days

7c25d4f

klopfdreh force-pushed the feature/cleanupEndTime branch from aa5dfb8 to 7c25d4f Compare August 30, 2023 08:41

onobc suggested changes Aug 30, 2023

View reviewed changes

klopfdreh added 4 commits August 31, 2023 06:21

fix: cleanup before n days review adjustments

b9b973f

fix: cleanup before n days review adjustments

c313a67

fix: cleanup before n days review adjustments

167a9ff

fix: cleanup before n days review adjustments

d4b2be3

fix: cleanup before n days review adjustments

98f69dd

klopfdreh requested a review from onobc August 31, 2023 05:15

fix: cleanup before n days review adjustments

1136dcc

onobc reviewed Aug 31, 2023

View reviewed changes

...ringframework/cloud/dataflow/aggregate/task/impl/AggregateDataFlowTaskExecutionQueryDao.java Outdated Show resolved Hide resolved

fix: cleanup before n days review adjustments

750aae2

onobc reviewed Sep 1, 2023

View reviewed changes

onobc pushed a commit to onobc/spring-cloud-dataflow that referenced this pull request Sep 4, 2023

Add num days to task cleanup.

b1ab1eb

See spring-attic#5422

onobc added a commit that referenced this pull request Sep 4, 2023

Add num days to task cleanup (#5453)

af60a25

* Add num days to task cleanup. See #5422 Co-authored-by: Tobias Soloschenko <tsoloschenko@apache.org>

onobc closed this Sep 4, 2023

onobc removed this from the 2.11.0 milestone Sep 4, 2023

Conversation

klopfdreh commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onobc commented Aug 29, 2023

Uh oh!

klopfdreh commented Aug 29, 2023

Uh oh!

klopfdreh commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onobc commented Aug 30, 2023

Uh oh!

klopfdreh commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onobc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

onobc commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klopfdreh commented Aug 31, 2023

Uh oh!

Uh oh!

onobc Sep 1, 2023

Choose a reason for hiding this comment

Case 1

Case 2

Uh oh!

klopfdreh Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onobc Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

onobc commented Sep 4, 2023

Uh oh!

onobc commented Sep 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

klopfdreh commented Aug 2, 2023 •

edited

Loading

klopfdreh commented Aug 29, 2023 •

edited

Loading

klopfdreh commented Aug 30, 2023 •

edited

Loading

onobc left a comment •

edited

Loading

onobc commented Aug 31, 2023 •

edited

Loading

klopfdreh Sep 1, 2023 •

edited

Loading