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

Fixes #18122 - move rhsm fact update to run phase #6554

Merged
merged 1 commit into from Jan 24, 2017

Conversation

jlsherrill
Copy link
Member

During fact importing, RhsmFactName objects are created
in large quantity. Because first_or_create is very susceptible
to race conditions, we rescue on RecordNotUnique within
RhsmFactImporter#add_fact_name. However if duplicate record
exception is thrown while in a transaction the entire
transaction is aborted. The run phase does not use a transaction
so the problem should not occur there.

@mention-bot
Copy link

@jlsherrill, thanks for your PR! By analyzing the history of the files in this pull request, we identified @thomasmckay, @bbuckingham and @bkearney to be potential reviewers.

host = ::Host.find(input[:host_id])
unless input[:facts].blank?
::Katello::Host::SubscriptionFacet.update_facts(host, input[:facts])
input[:facts] = 'TRIMMED'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part intentional? I didn't see it in the code that got moved into run

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, the purpose is so that we do not cause the foreman tasks tasks table to balloon. We do that in other places such as:

input[:consumer_params][:facts] = 'TRIMMED'

During fact importing, RhsmFactName objects are created
in large quantity. Because first_or_create is very susceptible
to race conditions, we rescue on RecordNotUnique within
RhsmFactImporter#add_fact_name.  However if duplicate record
exception is thrown while in a transaction the entire
transaction is aborted.  The run phase does not use a transaction
so the problem should not occur there.
@bbuckingham
Copy link
Member

I was not able to reproduce the issue that was initially reported; however, these changes didn't appear to have any negative impacts during my tests. ACK.

@bbuckingham bbuckingham self-requested a review January 24, 2017 20:16
@jlsherrill jlsherrill merged commit 98bb66d into Katello:master Jan 24, 2017
@jlsherrill jlsherrill deleted the run_facts branch January 24, 2017 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants