On 08/02/2012 02:38 PM, Imre Farkas wrote:
On 07/30/2012 11:23 AM, Jan Provaznik wrote:
> On 07/22/2012 12:16 PM, ifarkas(a)redhat.com wrote:
>> This patchset implements the initial framework
>>
>
> Hi Imre,
> this is not a proper review (Jozef is reviewing this patch AFAIK), here
> is a few things we discussed on Friday:
> - provider account's priority is not used when this patch is applied,
> it's replaced with priority groups. I'm fine with your suggestion that
> this account's priority can be used for setting wigh of account when
> choosing random account inside priority group.
> - provider_priority_groups controller is missing permissions checking
> for some actions
> - priority group is not destroyed when the pool is destroyed, priority
> element is not destroyed when account or provider is destroyed
> - config/initializers/provider_selection_strategies.rb: only
> lib/provider.rb could be loaded instead of the each loop
> - I wonder if priority groups shouldn't be associated with a pool family
> isntead of a pool since provider accounts are associated with pool
> family, not pool?
I think the ability to define priority groups on the pools level and
giving the ability to define a more complex provider selection logic is
plus. Imagine a situation where you have different pools for your
different environment (e.g. staging, production) and you want to want to
use providers with different availability/price ratio.
Does it make sense to you?
OK, makes sense
> - you use reverse priority ordering (bigger = higher) than what
is used
> for account's priority (lower = higher), we should unify this
>
> In future (post 1.1) it would be nice to make provider_selection library
> more abstract (conductor independent) which can be reused in other apps.
> The conductor specific logic would be just inside strategies.
>
> The above are mostly minor notes, overall I think this is a good stuff
> and it can be pushed before 1.1.
>
> Great job, Jan
The other issues are fixed in rev. 2 (except the plans for post 1.1).
Thanks for the review!
Imre