On Mon, 2012-07-30 at 11:23 +0200, Jan Provaznik wrote: > On 07/22/2012 12:16 PM, [email protected] 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? > - 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
I added some inlines to Jan's notices. It will be good if you send revision of the patchset to the mailing list. -- Jozef Zigmund
