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

Introduce report result purging timer #13044

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Dec 7, 2016

The resources required to purge report results used to be quite large.
So it was not scheduled

In many databases, report results is the second or third largest table (by memory usage)

before

customers manually purged the report results table

after

the report results table is checked once per week.

concerns

While these can be run by any worker, I am not sure which worker needs to schedule it.
So I put it under the schedules_for_scheduler_role

https://bugzilla.redhat.com/show_bug.cgi?id=1348625

@jrafanie
Copy link
Member

jrafanie commented Dec 8, 2016

The resources required to purge report results used to be quite large.
So it was not scheduled

In many databases, report results is the second or third largest table (by memory usage)

@kbrock Will users have to truncate or purge this table manually before this code is run? I could imagine the first scheduled one could hit the message timeout if the table hasn't been pre-purged before.

@kbrock
Copy link
Member Author

kbrock commented Dec 8, 2016

@jrafanie even if a user is purging for the first time and they choose 'purge all but 5 newest` (historically slower) - it should still only take a few minutes.

Aah, they are purging associated records, including blobs. this may take a bit. I'll play with this

@jrafanie
Copy link
Member

jrafanie commented Dec 8, 2016

@jrafanie even if a user is purging for the first time and they choose 'purge all but 5 newest` (historically slower) - it should still only take a few minutes.

Aah, they are purging associated records, including blobs. this may take a bit. I'll play with this

ok, yeah @kbrock, I was basing my comment on this:

The resources required to purge report results used to be quite large.
So it was not scheduled

If we have lots of results, is it possible to timeout the queue message...?

@kbrock
Copy link
Member Author

kbrock commented Dec 8, 2016

If we have lots of results, is it possible to timeout the queue message...?

We have cut down considerably the amount of data going over the wire and the amount of data that is being serialized.
This use case has associated tables, so there is still timing issues, but it should have cut down considerably

running tests to see how expensive it is to delete a lot

@kbrock
Copy link
Member Author

kbrock commented Dec 8, 2016

@jrafanie
ok ran some numbers. For 27,841 record in MiqReportResult it is taking 2.5 minutes. and 18,383,604 records in MiqReportResultDetails
Shouldn't be a problem.

@@ -1383,6 +1383,7 @@
:performance_rollup_purging_start_delay: 5.minutes
:policy_events_purge_interval: 1.day
:poll: 15.seconds
:report_result_purge_interval: 1.days
Copy link
Member Author

Choose a reason for hiding this comment

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

this may be too frequent. any suggestions would be great

Copy link
Member

Choose a reason for hiding this comment

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

Is 1.month too infrequent? If so, how is 7.days?

@kbrock
Copy link
Member Author

kbrock commented Dec 10, 2016

done. changed to 1/month

@@ -213,6 +213,12 @@ def schedules_for_scheduler_role
enqueue :miq_alert_evaluate_hourly_timer
end

# Schedule - Prune old reports Timer
every = worker_setting_or_default(:report_result_purge_interval, 1.day)
Copy link
Member

Choose a reason for hiding this comment

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

This default should probably match what's in the settings.yml

@@ -1383,6 +1383,7 @@
:performance_rollup_purging_start_delay: 5.minutes
:policy_events_purge_interval: 1.day
:poll: 15.seconds
:report_result_purge_interval: 1.months
Copy link
Member

Choose a reason for hiding this comment

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

The thing I don't like about 1.month is that if the server is restarted, the timer also restarts...not sure how often appliances are restarted.

@miq-bot
Copy link
Member

miq-bot commented Dec 14, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@kbrock
Copy link
Member Author

kbrock commented Dec 14, 2016

fixed merge conflict.
changed to 1/week.
no longer have differing defaults, since all comes from settings.yml

The resources required for report result purging has been reduced,
so make sure this is scheduled.

https://bugzilla.redhat.com/show_bug.cgi?id=1348625
@miq-bot
Copy link
Member

miq-bot commented Dec 14, 2016

Checked commit kbrock@2f78249 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

@@ -117,6 +117,10 @@ def policy_event_purge_timer
queue_work(:class_name => "PolicyEvent", :method_name => "purge_timer", :zone => nil)
end

def miq_report_result_purge_timer
queue_work(:class_name => "MiqReportResult", :method_name => "purge_timer", :zone => nil)
Copy link
Member

Choose a reason for hiding this comment

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

  • nil zone
  • schedules_for_scheduler_role

This means the singular regional "scheduler" role schedule worker will run this and queue it for the deletion of old report results for the whole region. If you were concerned with who should run it, I think the above confirms you did it right.

Copy link
Member

Choose a reason for hiding this comment

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

Note, I'm not sure how you could really test this cleanly

@jrafanie jrafanie added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 14, 2016
@jrafanie jrafanie merged commit 1bd9279 into ManageIQ:master Dec 14, 2016
@kbrock kbrock deleted the purge_report_results branch December 14, 2016 22:10
@simaishi
Copy link
Contributor

simaishi commented Jan 9, 2017

Euwe backport conflict:

$ git cherry-pick -e -x -m 1 1bd9279
error: could not apply 1bd9279... Merge pull request #13044 from kbrock/purge_report_results
$ git diff
diff --cc config/settings.yml
index 0f79392,8ed260b..0000000
--- a/config/settings.yml
+++ b/config/settings.yml
@@@ -1364,6 -1392,8 +1364,11 @@@
        :performance_rollup_purging_start_delay: 5.minutes
        :policy_events_purge_interval: 1.day
        :poll: 15.seconds
++<<<<<<< HEAD
++=======
+       :resync_rhn_mirror: 12.hours
+       :report_result_purge_interval: 1.week
++>>>>>>> 1bd9279... Merge pull request #13044 from kbrock/purge_report_results
        :server_log_stats_interval: 5.minutes
        :server_stats_interval: 60.seconds
        :service_retired_interval: 10.minutes

@simaishi
Copy link
Contributor

simaishi commented Jan 9, 2017

@kbrock Please resolve conflict and make Euwe-specific PR (referencing this one) or suggest other PRs to backport.

@simaishi
Copy link
Contributor

Backported to Euwe via #13429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants