Comment 32 for bug 1520719

In the drivers meeting we decided to adopt the context manager instead of the decorator over the existing code. Here is the discussion excerpt:

http://eavesdrop.openstack.org/meetings/neutron_drivers/2016/neutron_drivers.2016-08-11-22.14.log.html

22:18:30 <kevinbenton> HenryG, armax: I think it will get done a lot quicker if we use the context managers
22:18:46 <HenryG> I hope so, let's try that
22:18:47 <kevinbenton> using the decorators means the whole function has to be 'PRECOMMIT' so-to-speak
22:18:51 <armax> kevinbenton: it depends if that’s what zzzeek was proposing?
22:18:56 <kevinbenton> which means every one requires a refactor
22:19:24 <kevinbenton> armax: well the difference between the context manager and the decorator is just syntax
22:19:27 <armax> kevinbenton: yes, that’s something I questioned
22:19:33 <kevinbenton> they both use the new reader/writer thingy
22:19:44 <kevinbenton> so we still get the advantages of that
22:20:14 <HenryG> I don't know if zzzeek knew how many of our methods do things after commiting before they return
22:20:35 <kevinbenton> yeah, or did things before the transaction started
22:21:35 <kevinbenton> but really the difference between the decorator vs the context manager is just syntactic sugar
22:21:47 <kevinbenton> the important thing is switching to the new facade
22:23:42 <armax> as for the way we leverage it we can either go with what kevinbenton is suggesting
22:23:45 <armax> which seems less invasive
22:24:02 <armax> or go with what zzeeek was suggesting, which is more invasive?
22:24:40 <HenryG> it requires refactoring, yes
22:24:48 <kevinbenton> there is almost no benefit to using the decorator instead of the manager. the only thing i can see is that it makes it a little easier to reason about a function being entirely enclosed in a transaction
22:25:26 <kevinbenton> so even if we could do the decorator really quickly, i would be against it this late in the cycle due to all of the code churn it will cause
22:25:54 <HenryG> the "plan" would be 'search and replace' using the context manager
22:25:59 <armax> kevinbenton: we can’t simply apply the decorator blindly though, can we?
22:26:00 <kevinbenton> HenryG: +1
22:26:06 <kevinbenton> armax: that's my point
22:26:16 <kevinbenton> armax: decorator we can't apply without thinking and refactoring
22:26:21 <armax> kevinbenton: right, that goes back to the refactoring need, which is a mess
22:26:23 <kevinbenton> armax: context manager we pretty much can
22:26:42 <armax> do we lose any benefit if we used the context manager based approver over decorators?
22:26:52 <armax> then we can still make a mandate that new junk uses the decorator
22:26:57 <armax> can the two co-exist?
22:27:02 <kevinbenton> no different
22:27:11 <kevinbenton> they are just different entry points into the same class
22:27:14 <kevinbenton> i looked into it
22:27:16 <HenryG> Using the decorator forces a cleaner transaction design IMO
22:27:37 <armax> right, so if it’s cleaner/more readable to use the decorator then we can suggest using it during code review
22:27:45 <armax> and apply it where it’s easier to do so
22:27:51 <amuller> agreed
22:27:56 <amuller> it's two different efforts
22:28:04 <armax> ie. the whole method is easily wrappable
22:28:33 <kevinbenton> that becomes difficult when we want to emit PRECOMMIT and AFTER events
22:28:47 <armax> kevinbenton: understood
22:28:48 <kevinbenton> just have to break things down into more functions i suppose
22:28:54 <armax> correct
22:29:02 <HenryG> ack
22:29:05 <armax> we can apply this ad hoc
22:29:23 <armax> and going forward we can keep an eye on suggesting to use the decorator-based approach where it makes sense
22:29:41 <armax> for now, we should switch to the new facade everywhere where it’s needed
22:29:43 <armax> and be done with it
22:29:52 <HenryG> I think we agree
22:29:57 <armax> going forward we can go piecemeal
22:30:17 <armax> between a little review attention and janitorial duties in cleaning up the code slowly
22:30:25 <armax> as low-hanging-fruit type of an effort