Thanks for this most interesting and informative exchange. On Fri, Sep 8, 2017 at 5:20 PM, Michael Bayer <mba...@redhat.com> wrote: > On Fri, Sep 8, 2017 at 8:53 AM, Kevin Benton <ke...@benton.pub> wrote: > > Since the goal of that patch is to deal with deadlocks, the retry can't > > happen down at that level, it will need to be somewhere further up the call > > stack since the deadlock will put the session in a rollback state. > > > OK when I was talking to Michel about this, I first mentioned that to > get retry within this in-a-transaction block would require adding a > savepoint to the mix, but he seemed to indicate that the method can > also run by itself, but since there's no session.begin() there then > that matches with my initial impression that the retry has to be at > the level which the transaction starts, and that's not here.
I think there is also a deal of madness on how things are implemented in this particular project. I'll review the way it's handled and adjust accordingly, and then implement the right retries in the correct places. I think my strategy will be to first start with using a decorator based on wrap_db_retry to fix the bug. After that I'll start, in another set of patches, the migration from the "legacy" pattern to the "new" enginefacade. What do you think? Or given that enginefacade already diminishes the possibilities of deadlocks I should already start with the migration to enginefacade and then with that already in place > > For the functions being called outside of an active session, they should > > switch to accepting a context to handle the enginefacade switch. ACK. > >> 3a. Is this a temporary measure to work around legacy patterns? > > > > Yes. When that went in we had very little adoption of the enginefacade. Even > > now we haven't fully completed the transition so checking for is_active > > covers the old code paths. > > OK, so this is where they are still calling session.begin() > themselves. Are all these session.begin() calls in projects under > the "openstack" umbrella so I can use codesearch ? So, to make sure I understand correctly. With enginefacade instead of calling session.begin() to handle the transactions, when using 'reader' o 'writer' we are already inside a transaction, right? If that's correct, no matter if decorating or using a context manager, in both cases the transaction lives for the context of the decorated function or the with-stmt context, right? > > As we continue migrating to the enginefacade, the cases where you can get a > > reference to a session that actually has 'is_active==False' are going to > > keep dwindling. Once we complete the switch and remove the legacy session > > constructor, the decorator will just be adjusted to check if there is a > > session attached to the context to determine if retries are safe since > > is_active will always be True. Therefore, on implementation of enginefacade retry_if_session_inactive is rendered useless and because of the deepcopy retry_db_errors in our usecase is already useless. As a result we are better off with using wrap_db_retry to create a decorator parametrised according to our needs (ie: max retries, exception checker, etc), right? And for exception checker I should use the is_retriable from neutron-lib instead of the neutron db api, right? (it's a shame that MAX_RETRIES is not also on neutron-lib and only in neutron) Thanks to you both. __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev