[Ovirt-devel] [PATCH server] controller service layer refactoring for pools.

[Scott Seago]

David Lutterkort wrote:
> On Wed, 2009-04-29 at 16:50 -0400, Scott Seago wrote:
>   
>   
>>> Why not make throwing PartialSuccessError part of the method's
>>> signature ? Does wrapping in an ActionError really buy us anything ?
>>>
>>>   
>>>       
>> I'm wrapping it since I'm rethrowing it within a transaction -- causing 
>> an abort.
>>     
>
> My main objection was that by rethrowing a different exception we
> obscure the actual failure for the caller and make it harder to create a
> precise error message - this would actually all be moot if we made
> PartialSuccessError a subclass of ActionError.
>
>   
I guess I'm not sure what the right solution is. On one hand we don't 
want to lose information on what was  thrown originally. On the other 
hand PartialSuccessError gives the impression from its embedded 
successes/failures attributes that part of the action succeeded. In this 
particular case, though, where these multi-object actions are executed 
as part of a create operation, we're explicitly aborting the 
transaction, so passing the PartialSuccessError up would _also_ give a 
false impression, namely that error.successes did indeed get committed 
to the database. This is starting to sound a bit absurd, but we could 
always create an AbortedPartialSuccessError (possibly extending 
ActionError) class that contains a reference to the thrown 
PartialSuccessError exception. That would resolve the exception 
swallowing inaccuracies _and_ the no-longer-accurate partial success 
notification, but it's not the most elegant solution.
>>>> +      end
>>>> +    end
>>>> +    return alert
>>>> +  end
>>>> +
>>>> +  def svc_move_hosts(pool_id, host_ids, target_pool_id)
>>>> +    svc_move_items_internal(pool_id, Host, host_ids, target_pool_id)
>>>> +  end
>>>> +  def svc_move_storage(pool_id, storage_pool_ids, target_pool_id)
>>>> +    svc_move_items_internal(pool_id, StoragePool, storage_pool_ids, target_pool_id)
>>>> +  end
>>>> +  def svc_move_items_internal(pool_id, item_class, resource_ids, target_pool_id)
>>>> +    # from before_filter
>>>> +    @pool = HardwarePool.find(pool_id)
>>>> +    target_pool = Pool.find(target_pool_id)
>>>> +    authorized!(Privilege::MODIFY,target_pool)
>>>> +    authorized!(Privilege::MODIFY, at pool) unless @pool == target_pool
>>>>     
>>>>         
>>> Does it even make sense to do anything if @pool == target_pool ? We
>>> should just bail.
>>>
>>>   
>>>       
>> We shouldn't bail, since for the "move hosts _to_ this pool" operation 
>> they will be the same -- the unless clause keeps us from making the 
>> check twice. For the "move hosts _to another_ pool" case the two will be 
>> different.
>>     
>
> Ahh, ok, that makes sense (though the unless clause is unnecessary with
> the changes to set_perms that are also in the patch)
>
>   
True. the set_perms change happened later in the iteration of  this 
re-work. I'll go back and verify that it is now unnecessary and pull it 
assuming  that the set_perms change does the same thing now.

Scott
> David
>
> [1] http://book.git-scm.com/4_interactive_rebasing.html
>
>
>