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

put S3 refresh in a separate worker #17704

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

AlexanderZagaynov
Copy link
Contributor

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

Put S3 refresh in a separate worker, as it can contain too many objects, which blocks other data refresh.

Coupled PR: ManageIQ/manageiq-providers-amazon#461

@AlexanderZagaynov
Copy link
Contributor Author

@Ladas @tumido please review
(P.S. I have no idea how to spec that)

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

👍 looks good

I don't think we have specs for this file

@Ladas
Copy link
Contributor

Ladas commented Jul 16, 2018

@miq-bot assign @agrare
@miq-bot add_label enhancement, performance

@agrare
Copy link
Member

agrare commented Jul 16, 2018

@Ladas we moved the child managers into a single worker because of races with targeted refresh right? Is that not a concern with the S3 manager?

@Ladas
Copy link
Contributor

Ladas commented Jul 16, 2018

@agrare should not be, S3 entities are not connected to anything from the other managers, nor there is a targeted refresh for it. (I believe it was the targeted refresh doing the race, since it would be 2 workers saving the same records)

@juliancheal
Copy link
Member

@agrare Yeah should be ok :)

@agrare
Copy link
Member

agrare commented Jul 16, 2018

Okay thanks guys, I'm good with this then. I assume this has to be merged right before the amazon PR. When you two approve it I'll merge both together.

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

🎉 🎆 LGTM

@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2018

Checked commit AlexanderZagaynov@0c82f49 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@agrare agrare merged commit 0c82f49 into ManageIQ:master Aug 1, 2018
agrare added a commit that referenced this pull request Aug 1, 2018
put S3 refresh in a separate worker
@agrare agrare added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 1, 2018
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.

7 participants