Move EmsRefresh targets to MiqQueue data #16271
Conversation
agrare
force-pushed
the
move_ems_refresh_targets_to_miq_queue_data
branch
2 times, most recently
from
e286603 to
b500ff9
Compare
| @@ -171,7 +171,7 @@ def self.queue_merge(targets, ems, create_task = false) | |||
|
|
|||
| # Items will be naturally serialized since there is a dedicated worker. | |||
| MiqQueue.put_or_update(queue_options) do |msg, item| | |||
| targets = msg.nil? ? targets : (msg.args[0] | targets) | |||
| targets = msg.nil? ? targets : msg.data.concat(targets) | |||
There was a problem hiding this comment.
gives undefined method `concat' for nil:NilClass (NoMethodError)
we need to do
(msg.data || []).concat(targets)There was a problem hiding this comment.
Hm that's strange I didn't get this, what case did you hit that? That would have to be if the message existed but didn't get any targets assigned?
There was a problem hiding this comment.
this happens to me everytime that new MiqQueue record is created, the default data seems to be nil
There was a problem hiding this comment.
If you have a new MiqQueue record, then msg will be nil and you'll hit the first part of the ternary.
There was a problem hiding this comment.
I think this happens when merging existing queue items where msg isn't nil but data hasn't been set, also can go away of we migrate existing queue items
There was a problem hiding this comment.
Migration sounds like the best option 👍
app/models/ems_refresh.rb
Outdated
| unless msg.args[0].nil? | ||
| msg.data.concat(msg.args.shift) | ||
| msg.args = [] | ||
| end |
There was a problem hiding this comment.
@Fryguy this works around merging targets into existing queue items that still had their targets in args, do you think this is needed?
agrare
force-pushed
the
move_ems_refresh_targets_to_miq_queue_data
branch
from
b044dd7 to
a365926
Compare
agrare
force-pushed
the
move_ems_refresh_targets_to_miq_queue_data
branch
3 times, most recently
from
e964b48 to
53e608b
Compare
app/models/ems_refresh.rb
Outdated
| if msg && msg.args[0] | ||
| msg.data.concat(msg.args.shift) | ||
| msg.args = [] | ||
| end |
There was a problem hiding this comment.
@Fryguy made some changes so my comment was removed but same question here ^^ do you think this is needed? My worry is if we don't clear args then we'll get refresh delivered with EmsRefresh.refresh(args, data)
There was a problem hiding this comment.
I'm ok with this for now, but ultimately, I don't think it's needed, or we should deal with it in a data migration. I don't like live "fixing" of data.
There was a problem hiding this comment.
Okay I'll work on a data migration so we can get rid of this, agree we don't want to carry this forever
There was a problem hiding this comment.
Can be removed once ManageIQ/manageiq-schema#107 is in
app/models/ems_refresh.rb
Outdated
| msg.args = [] | ||
| end | ||
|
|
||
| targets = msg.nil? || msg.data.nil? ? targets : msg.data.concat(targets) |
There was a problem hiding this comment.
Same here, this can go back to msg.nil? ? targets : ... once we get ManageIQ/manageiq-schema#107 in
Reduce the overhead of put_or_update by moving EmsRefresh targets from MiqQueue args to data. Also replace uniquing the targets on put with `msg.args[0] | targets` with just a simple concat and unique the targets on the dequeue side.
agrare
force-pushed
the
move_ems_refresh_targets_to_miq_queue_data
branch
from
53e608b to
3691157
Compare
|
Okay with ManageIQ/manageiq-schema#107 merged I just dropped the last commit since all it did was handle migrating queue args. |
|
Checked commit agrare@3691157 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@Fryguy can you take another look? Should be ready now. |
Reduce the overhead of put_or_update by moving EmsRefresh targets from
MiqQueue args to data. Also replace uniquing the targets on put with
msg.args[0] | targetswith just a simple concat and unique the targetson the dequeue side.