MiqQueue#put_unless_exists supports create_with

[kbrock]

Summary

MiqQueue#put_unless_exists adds a unique message to the MiqQueue.

This PR change:

• Make it easier to declare which parameters are used for uniqueness (the _unless_exists part) and which are used for creation (put part).
• Prep for: change the SQL SELECT, INSERT queries to a single INSERT statement. (all data in sql friendly hashes, and not looking at returning object)

Before

We used a block that was clunky, nuanced, and preventing us from tuning the sql:

MiqQueue.put_unless_exists(
  :class_name  => "Metric",
  :method_name => "purge_hourly",
) do |_msg, find_options|
  find_options.merge(:args => [timestamp])
end

What does this do?

• If a message for Metric#purge_hourly already exists, then don't create one. Ignore the :args value since it is a timestamp and probably won't match anyway.
• If a message for Metric#purge_hourly does not already exist, then create one. The new record will include the :args value provided.

Stated another way:

• :class_name and :method_name are part of the uniqueness constraint.
• :class_name, :method_name, and :args are used for creation.

After

We can leverage an active record mechanism created_with to convert these parameters into SQL. On one hand, it does not introduce any change to MiqQueue, but it is more closely coupled with ActiveRecord:

MiqQueue.create_with(:args => [timestamp]).put_unless_exists(
  :class_name  => "Metric",
  :method_name => "purge_hourly"
)

UPDATE: This PR only updates MiqQueue - ensuring that it works with create_with for the parameters of interest

related: #14676

[kbrock]

This is starting scope creep - pulled out last commit into own PR (14569). this should simplify it (at least a little)

[gtanzillo]

I'm comfortable with this new pattern. @kbrock are going to change all the places where put_unless_exists is being used?

[kbrock]

ugh - I had this in a different PR

[kbrock]

@gtanzillo Of the 31 instances of put_unless_exist, 8 took a block. Of these 8, 5 manipulated the hash in the block. This pr changes all 5 to not manipulate the block:

#before
MiqQueue.put_unless_exists(unique_params) do |msg|
  msg.merge(create_params)
end
# after
MiqQueue.create_with(create_params).put_unless_exists(unique_params)
# or
MiqQueue.create_with(create_params).put_unless_exists(unique_params) do |msg|
  _logger.debug("created a record #{msg.id}")
end

There are some put_or_update that don't manipulate the queue entries. So in #14569 I changed them over to create_with().put_unless_exist().

[kbrock]

WIP:

• purging goes into separate pr / and becomes just a put
• clean up extra commits that got into here
• ensure callbacks don't get botched up

[kbrock]

fixed cops - I'll pull out anything that isn't straight forward

[NickLaMuro]

Only really have one comment regarding to code, and it really is just asking for a clarification on the intent of the PR.

Overall, I think this is a fine way to do things going forward, and will (I assume) optimize things in the future when it is officially implemented. That said, while it doesn't make the interface any worse than what we had (I would agree with you that it is an improvement), the interface is still a bit confusing in the sense that it still requires two method calls to do what I think is perceived as an "atomic action".

Again, don't have a better solution than what you have here, and more bringing it up as it might still be a somewhat confusing mechanism.

[NickLaMuro]

Seems like this is the only actual code change to the application code, and effectively it was just removing something that was nil regardless, right?

[kbrock]

Right, this is the only real change.

Added a slew of tests to ensure nothing was broken

[kbrock]

And to be honest, this is defaulting deliver_on to a nil (which ruby already does)

[kbrock]

In ActiveRecord, where(:a => 5).order(:c) is 2 method calls but an atomic method action.
ActiveRecord does create_with(:c => 5).create_or_update(:a => 5) as an atomic concept.

True, current implementation has the second as 2 different database queries, but that is what monkey patching / PRs is about

[NickLaMuro]

ActiveRecord does create_with(:c => 5).create_or_update(:a => 5).

@kbrock That is fair. I guess that goes to show you my lack of keeping up with the new ActiveRecord interfaces. Anyway, looks good.

[miq-bot]

Checked commits kbrock/manageiq@b713984~...7c464d1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

[Fryguy]

@kbrock Please also don't forget to check for put_unless_exists in the split out repos. Not sure they exist or not, but we can't forget those.