Lonnen :lonnen Reporter
	Description • 6 years ago

Some connection classes have no defined conditional errors, resulting in an empty tuple as a class property, which is passed to an except statement in the transaction executor. Python 2 will catch any old class, but Python 3 only catches classes derived from BaseException. Empty tuples cause errors. Fix it.

	Will Kahn-Greene [:willkg] ET needinfo? me Assignee
	Comment 1 • 6 years ago

Lonnen and I talked about this. The TransactionExecutor classes all cover two things:

1. retrying
2. performing transaction-related bookkeeping like commit and rollback at appropriate times

It's possible we don't need all that functionality anymore. We should look into that.

If we do need it, we should simplify this greatly and push exception identification to the connection context. The code should be half as big as it is. We can probably nix all the subclass variations, too.

Grabbing this to work on soon.

	Will Kahn-Greene [:willkg] ET needinfo? me Assignee
	Comment 2 • 6 years ago

The thing that uses this the most is the postgres stuff. We're pretty close to nixing that altogether. Once that's gone, the idea of "transactions" is not helpful for the other things that are using it since they don't have any concept of rolling back or committing.

Given that, it's probably the case that instead of rewriting or fixing this, we'll just remove it.

I'm going to close this as WONTFIX with the expectation that this will go away when we purge everything in socorro/external/postgres/.

	Will Kahn-Greene [:willkg] ET needinfo? me Assignee
	Comment 3 • 6 years ago

It didn't go away. Reopening this to deal with it.

	Will Kahn-Greene [:willkg] ET needinfo? me Assignee
	Comment 4 • 6 years ago

The TransactionExecutors do two things:

1. wrap connection contexts in transaction followup code like "commit" and "rollback" (commit/rollback)

2. close and recreate connections in certain circumstances (recycle connection)

3. retry operations in certain known no-big-deal failure cases (retry)


We use these in three situations:

1. when doing things with postgres (commit/rollback, retry)
2. when doing things with ES (retry)
3. when doing things with rabbitmq (recycle connection, retry)
4. when doing things with boto (retry)


I'm going to write these:

1. a transaction_context context manager
2. a retry function that recycles connections on failures

then rework the code to use those.

There are tons of retry implementations out there, but we just need something small and minimally featured plus I want to make sure to incorporate the quit_check otherwise everything gets all grumpy. I'm going to roll my own.

	[github robot]
	Comment 6 • 6 years ago

Commits pushed to master at https://github.com/mozilla-services/socorro

https://github.com/mozilla-services/socorro/commit/dcf2dbc97fd090b3f7c96cbd72203b918d1b1a85
fix bug 1503434: redo retries and transactions

Socorro has a family of TransactionExecutor classes which perform transaction
bookkeeping and also retry failed operations. The way it's implemented doesn't
work with Python 3. Rather than fix the issues, this splits the
TransactionExecutor into a retry function and a transaction_context context
manager and then redoes everything to use those.

This does a couple of things:

1. simplifies executing operations with transaction bookkeeping requirements
   so they're easier to reason about

2. simplifies retriable operations so they're easier to reason about

3. gets rid of one or both in places that don't need them

https://github.com/mozilla-services/socorro/commit/7a072ab98cfe4fa294d675d9874f617a063294c2
Merge pull request #4727 from willkg/1503434-transaction

fix bug 1503434: redo retries and transactions