Skip to content
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

Execution plan cleaner #244

Merged
merged 2 commits into from Aug 18, 2017
Merged

Execution plan cleaner #244

merged 2 commits into from Aug 18, 2017

Conversation

adamruzicka
Copy link
Contributor

It is disabled by default, needs to be enabled in a world's config. For the usage on smart-proxy this should be good to go since there's mostly one or two kinds of tasks (rex and ansible), so we probably don't need fine grained controls for setting different interval for different actions.

Copy link
Member

@iNecas iNecas left a comment

Choose a reason for hiding this comment

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

Looks good code-wise, except one potential issue. Pending tests.


def clean!
plans = @world.persistence.find_old_execution_plans(Time.now.utc - @max_age)
@world.persistence.delete_execution_plans(uuid: plans.map(&:id))
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this have the issue with the blank filer on uuid => [] matching all the plans? I guess we should probably just fix this case directly in sequel persistence adapter, to avoid this issues in once and for all, as I don't see where it would actually be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tried it, looks it is safe to use now

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sry, my bad, I was trying it against find_execution_paln with wrong filter

@adamruzicka
Copy link
Contributor Author

Added some tests, let's see how Mr. T likes them

@adamruzicka
Copy link
Contributor Author

The tests are green

@iNecas
Copy link
Member

iNecas commented Aug 18, 2017

Tests speak for themselves: thanks @adamruzicka. Merging now

@iNecas iNecas merged commit e8518e2 into Dynflow:master Aug 18, 2017
@adamruzicka adamruzicka deleted the periodic-cleanup branch August 21, 2017 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants