Open Stack

On 2013-08-22 00:22, Morgan Fainberg wrote:
> I've been doing some pondering on how Keystone handles the various
> pluggable systems with it's Manager / Driver architecture.
> 
> Currently we implement the base driver class as follows:
> 
> There is a driver object that has a number of reference functions
> defined on it, each of these functions typically raises
> NotImplemented() and has a docstring describing the expected behavior.
>  A simple example of this would be the Token Driver base class.  A
> complex example would be the Identity Driver base class.
> 
> Example:
> 
>> class AbstractBaseClassTest(object):
> 
>>     def abstract_method1(args, kwargs):
> 
>>         raise NotImplemented()
> 
>>     def abstract_method2(args, kwargs):
>>         ...
> 
> This type of system is not inherently bad, nor flawed, but there are
> some cases when I've run into situations that raise a NotImplemented()
> error unexpectedly (usually in custom code I've had to write around a
> driver or replace a driver with and "internal to my company"
> implementation).
> 
> For those not familiar with ABCMeta, abstract base classes, and the
> abc module: 
> 
> In a model that uses an abstract metaclass, the base class methods
> that need to be overridden are decorated with the "abstractmethod"
> decorator from the "abc" module.  This decorator when coupled with
> the ABCMeta metaclass, requires all abstract methods to be overridden
> by the class that inherits from the base class before the class can be
> instantiated.  Similarly there is an "abstractproperty" decorator for
> @property methods.  
> 
> The exception raised looks like: 
> 
>> TypeError: Can't instantiate abstract class TestClass with abstract
>> methods AbsTest1, AbsTest2
> 
> The benefits are two fold.  First, this means that a new driver could
> not be implemented without overriding the expected methods (no raising
> "NotImplemented()" unintentionally) and it guarantees that even
> seldom-used expected functions would be defined on the driver
> implementations.  Second benefit is that these abstract methods can
> be called via the standard super() call, therefore if there is some
> base functionality that should be used (or legitimately you want to
> raise NotImplemented()), it is possible to have that defined on the
> parent class.
> 
> Example abstract base class (with using six module instead of directly
> setting __metaclass__):
> 
>> class AbstractBaseClassTest(six.with_metaclass(abc.ABCMeta)):
>>     @abc.abstractmethod
>>     def abstract_method1(int_arg):
>>         # we can do something here instead of raising
>>         # NotImplemented()
>>         return (int_arg + 1)
> 
> On to the downsides, the compatibility between python2.7 and python3k
> (using the six library) is not the most pleasing thing to look at.
>  It requires defining the object that inherits from a method call
> six.with_metaclass.  I also have not looked at the performance
> implications of this, however, at first blush, it doesn't look like
> should be significant.  There are possibly other pitfalls that
> haven't come to mind as of yet.
> 
> In short, the drivers should probably be actual abstract classes,
> since that is what they effectively are.  I've seen this
> functionality used in both Neutron and Ironic.  I could see it
> providing some benefits from it in Keystone.  I wanted to get the
> general feeling from the Mailing List on using abstracted base classes
> in lieu of our current implementation prior to proposing a Blueprint,
> etc.  I don't see this as a Havana implementation but possibly
> something to consider for Icehouse.
> 
> I don't know if the benefits really would bring us a huge win in
> Keystone, but I think it would make understanding what should be
> implemented when subclassing for driver development (and similar
> pluggable systems) a bit more clear.

I'm always a fan of pushing error discovery earlier (class 
instantiation) rather than later (method call).  So a big +1 from me.

-Ben