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

[V2V] Throttler - Replace class constant with global setting #18539

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2019

ManageIQ/manageiq#18528 introduced new keys in Settings.transformation.limits.max_concurrent_tasks_per_ems. This PR updates the migration throttler to use it instead of a class constant.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1688951

@ghost ghost changed the title Throttler Replace class constant with global setting [V2V] Throttler - Replace class constant with global setting Mar 10, 2019
@ghost
Copy link
Author

ghost commented Mar 10, 2019

@miq-bot add_label transformation, enhancement, hammer/yes
@miq-bot add_reviewer @djberg96
@miq-bot add_reviewer @jameswnl
@miq-bot add_reviewer @agrare

@miq-bot
Copy link
Member

miq-bot commented Mar 10, 2019

Checked commit fabiendupont@e62d533 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 self-assigned this Mar 11, 2019
@agrare agrare merged commit e62d533 into ManageIQ:master Mar 11, 2019
agrare added a commit that referenced this pull request Mar 11, 2019
…x_concurrent_tasks_per_ems

[V2V] Throttler - Replace class constant with global setting
@agrare agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 11, 2019
@agrare
Copy link
Member

agrare commented Apr 1, 2019

@fdupont-redhat this seems unrelated to the conversion host BZ

@ghost
Copy link
Author

ghost commented Apr 1, 2019

@agrare It doesn't have to be backported. Wrong copy/paste of labels.

@ghost
Copy link
Author

ghost commented Apr 1, 2019

@miq-bot remove_label hammer/yes

@miq-bot miq-bot removed the hammer/yes label Apr 1, 2019
@mturley
Copy link
Contributor

mturley commented Apr 10, 2019

@fdupont-redhat @jameswnl I think this PR needed to be hammer/yes because it consumes the setting added in #18528 and added to the UI in ManageIQ/manageiq-v2v#897, which were both hammer/yes. I'm not sure which BZ it should be attached to, though. It's causing problems for QE in verifying https://bugzilla.redhat.com/show_bug.cgi?id=1693746

@jameswnl
Copy link
Contributor

I am a little outdated as to whether the major throttling rework pr is backed ported or not (It is backported, but is the backport reverted later?)
If that is back ported, then yes, this PR should be as well.

@mturley
Copy link
Contributor

mturley commented Apr 10, 2019

If this one is not to be backported, we should revert the UI change in ManageIQ/manageiq-v2v#897. As it stands right now the UI setting for max migrations per provider has no effect (on hammer).

Looking through the commit log at https://github.com/ManageIQ/manageiq/commits/hammer, I see #18415 backported on March 8, and I don't see that PR number mentioned in any later commits. Seems backported to me?

@mturley
Copy link
Contributor

mturley commented Apr 10, 2019

@miq-bot add_label hammer/yes

simaishi pushed a commit that referenced this pull request Apr 10, 2019
…x_concurrent_tasks_per_ems

[V2V] Throttler - Replace class constant with global setting

(cherry picked from commit 5b6e5c5)

https://bugzilla.redhat.com/show_bug.cgi?id=1693746
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 59d5d163e7e945518d0aef81734f7574d48fe1c9
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Mar 11 09:25:34 2019 -0400

    Merge pull request #18539 from fdupont-redhat/v2v_use_settings_for_max_concurrent_tasks_per_ems
    
    [V2V] Throttler - Replace class constant with global setting
    
    (cherry picked from commit 5b6e5c56e76e3d928846a3f4078c84dcf1804bc5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1693746

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.

6 participants