MiqQueue consistency with purging #14676

kbrock · 2017-04-06T14:58:46Z

related to #14569

Bring consistency to Purging:

remove boilerplate
removing inconsistency (slowly)
moving all from put_unless_exists over to put.
put into the general queue
following patterns (which are already tuned for performance)

Fryguy · 2017-04-06T21:10:58Z

app/models/mixins/purging_mixin.rb

+#      mode  = value.number_with_method? ? :date : :remaining
+#      value = value.to_i_with_method.seconds.ago.utc if mode == :date
+#      return mode, value
+#   end


This comment is confusing to me.

Fryguy · 2017-04-06T21:12:10Z

app/models/mixins/purging_mixin.rb

 module PurgingMixin
  extend ActiveSupport::Concern

  module ClassMethods
+    # This provides backward support for date only implementations
+    # It is assumed most classes will override to provide `:remaining` support


If most classes will override, then why not make that the default and only do the rare places for override.

Even so, I think this comment is wrong, and I can't understand it. Everything supports by date, so why does it say "backward support for date only implementations". Nearly all of them are date only, right?

no, just under half are mode and date (options: :remaining, :date)

Fryguy · 2017-04-06T21:27:12Z

app/models/metric/purging.rb

@@ -35,15 +35,14 @@ def self.purge_realtime_timer(ts = nil)
    purge_timer(ts, "realtime")
  end

+  # NOTE: purge_#{interval} was introduced to make uniqueness easier
+  # now that we do not require unique purging, possibly go back to :args => [value, interval]


I feel like these NOTEs to yourself (as well as the others in this PR) would be better served if moved into a separate github issue.

following PurgingMixin framework - removed limit support, and manually overriding window_size - kept purge_date defaulting (to prevent oops and deleting everything)

converted put_or_update to put on general queue queuing directly into purge_by_{mode}

- converted put_or_update to put on general queue - queuing directly into purge_by_{mode} - delete already defined purge_timer

- converted put_or_update to put on general queue - queuing directly into purge_by_date

- converted put_or_update to put on general queue - allow multiple inserts into the queue NOTE: this is still not a typical purger

- converted put_or_update to put on general queue - introduce purge_by_date concept (which is just naming) NOTE: this is still not a typical purger

miq-bot · 2017-04-07T12:57:51Z

Checked commits kbrock/manageiq@d7a1e3a~...ecb49ef with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
13 files checked, 0 offenses detected
Everything looks good. 🏆

lfu · 2017-04-07T19:03:57Z

app/models/mixins/purging_mixin.rb

+    end
+
+    def purge_queue(mode, value)
+      MiqQueue.put(


@kbrock Role is not specified any more. Is it by intention?

Purging is quick. We perform it as pure sql and do not download tons of records.

So the thought is there is no reason to limit which workers can purge.
But that is my though, wanted your feedback here.

Agreed, purging is a pure database function and can be done anywhere

jrafanie · 2017-04-07T21:48:29Z

app/models/policy_event/purging.rb

-        )
-      end
-      alias_method :purge_timer, :purge_queue
-


Hmm, is the commit message wrong here?

- converted put_or_update to put on general queue - queuing directly into purge_by_date

Are we now using the mixin's method which does a put? All I see is deleted lines here, no new method.

It's ok, it's coming from the mixin. It wasn't clear when looking at the commit message.

thanks for keeping me honest.

Yes, most of these come from the default implementation

jrafanie · 2017-04-07T21:51:21Z

This approach looks good to me. Is it possible to have generic tests that each of these classes can be tested with shared examples?

Fryguy · 2017-04-10T21:54:19Z

@kbrock Is this for fine?

zeari · 2017-04-12T13:00:37Z

@kbrock Is this for fine?

Any reason it shouldnt be in fine?
Its needed for:
https://bugzilla.redhat.com/show_bug.cgi?id=1428595
#14322

kbrock · 2017-04-12T18:31:48Z

ok, fine/yes sounds fine

MiqQueue consistency with purging (cherry picked from commit 932ac57)

simaishi · 2017-04-13T18:34:00Z

Fine backport details:

$ git log -1
commit 9283c5a8a75ac449970f05752790527a7c6f98b2
Author: Jason Frey <[email protected]>
Date:   Mon Apr 10 17:54:05 2017 -0400

    Merge pull request #14676 from kbrock/miq_queue_purge_es
    
    MiqQueue consistency with purging
    (cherry picked from commit 932ac5773fcefa6dae3bc1234d95241549147af1)

kbrock requested review from jrafanie, Fryguy and lfu April 6, 2017 14:58

chessbyte changed the title ~~Miq queue purge es~~ MiqQueue purge EventStream Apr 6, 2017

chessbyte assigned gtanzillo Apr 6, 2017

Fryguy assigned Fryguy and unassigned gtanzillo Apr 6, 2017

kbrock force-pushed the miq_queue_purge_es branch from 7cbd421 to 4e25b61 Compare April 6, 2017 19:32

kbrock changed the title ~~MiqQueue purge EventStream~~ MiqQueue consistency with purging Apr 6, 2017

kbrock added core refactoring labels Apr 6, 2017

kbrock force-pushed the miq_queue_purge_es branch 2 times, most recently from 5799585 to 4aa5eaf Compare April 6, 2017 20:08

Fryguy reviewed Apr 6, 2017

View reviewed changes

kbrock added 7 commits April 7, 2017 08:44

PurgingMixin gets purge_timer and others

d7a1e3a

EventStream::Purging fixes

d9d1942

following PurgingMixin framework - removed limit support, and manually overriding window_size - kept purge_date defaulting (to prevent oops and deleting everything)

DriftState purging

7e80f6c

converted put_or_update to put on general queue queuing directly into purge_by_{mode}

MiqReportResult::Purging normalization

28c97a9

- converted put_or_update to put on general queue - queuing directly into purge_by_{mode} - delete already defined purge_timer

PolicyEvent::Purging normalization

ce61ca2

- converted put_or_update to put on general queue - queuing directly into purge_by_date

VmdbMetric::Purging normalization

a84c909

- converted put_or_update to put on general queue - allow multiple inserts into the queue NOTE: this is still not a typical purger

Metric::Purging normalization

ecb49ef

- converted put_or_update to put on general queue - introduce purge_by_date concept (which is just naming) NOTE: this is still not a typical purger

kbrock force-pushed the miq_queue_purge_es branch from 4aa5eaf to ecb49ef Compare April 7, 2017 12:53

kbrock mentioned this pull request Apr 7, 2017

Add purge timer for archived entities #14322

Merged

lfu reviewed Apr 7, 2017

View reviewed changes

jrafanie reviewed Apr 7, 2017

View reviewed changes

Fryguy merged commit 932ac57 into ManageIQ:master Apr 10, 2017

Fryguy added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 10, 2017

kbrock added the fine/no label Apr 10, 2017

kbrock deleted the miq_queue_purge_es branch April 10, 2017 22:06

kbrock added fine/yes and removed fine/no labels Apr 12, 2017

simaishi pushed a commit that referenced this pull request Apr 13, 2017

Merge pull request #14676 from kbrock/miq_queue_purge_es

9283c5a

MiqQueue consistency with purging (cherry picked from commit 932ac57)

simaishi added fine/backported and removed fine/yes labels Apr 13, 2017

kbrock mentioned this pull request Apr 20, 2017

[WIP]Use create_with vs use blocks for miq_queue updates #14569

Closed

1 task

This was referenced Apr 27, 2017

MiqQueue#put_unless_exists supports create_with #14562

Merged

LogManagement#delete_active use put_unless_exists #14926

Merged

kbrock mentioned this pull request May 4, 2017

[EUWE] partial Add purge timer for archived entities #15008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MiqQueue consistency with purging #14676

MiqQueue consistency with purging #14676

kbrock commented Apr 6, 2017 •

edited

Loading

Fryguy Apr 6, 2017

Fryguy Apr 6, 2017

kbrock Apr 7, 2017

Fryguy Apr 6, 2017

miq-bot commented Apr 7, 2017

lfu Apr 7, 2017

kbrock Apr 7, 2017

Fryguy Apr 10, 2017

jrafanie Apr 7, 2017

jrafanie Apr 7, 2017

kbrock Apr 7, 2017

jrafanie commented Apr 7, 2017

Fryguy commented Apr 10, 2017

zeari commented Apr 12, 2017

kbrock commented Apr 12, 2017

simaishi commented Apr 13, 2017

MiqQueue consistency with purging #14676

MiqQueue consistency with purging #14676

Conversation

kbrock commented Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Apr 7, 2017

Fryguy commented Apr 10, 2017

zeari commented Apr 12, 2017

kbrock commented Apr 12, 2017

simaishi commented Apr 13, 2017

kbrock commented Apr 6, 2017 •

edited

Loading