fix: don't rollback non-spanner connections on reset #709
Conversation
This is the simplest way to address bug googleapis#706 and address the `AttributeError: 'Connection' object has no attribute 'rollback'` I'm seeing when connections from BigQuery engines get caught by this event handler. I think we should be able to rely on SQLAlchemy's [reset on return](https://docs.sqlalchemy.org/en/20/core/pooling.html#reset-on-return) behaviour to rollback transactions when they're reset rather than call rollback ourselves. That behaviour is also something end users can configure, so it'd be good to respect their settings if they disable the behaviour. Fixes: googleapis#706
|
@waltaskew small nit regarding commit messages / pull request titles: These are used to automatically trigger releases and generate the release notes. For that, we use Conventional Commits. The prefix that should be used for bug fixes is |
There was a problem hiding this comment.
I agree that Spanner should not do anything with non-Spanner connections. I've manually verified that the default behavior of SQLAlchemy is to rollback on reset. That default is being executed after this event handler has been executed, meaning that non-Spanner connections get two rollback requests.
Got it -- sorry about that! |
This is the simplest way to address bug #706 and address the
AttributeError: 'Connection' object has no attribute 'rollback'I'm seeing when connections from BigQuery engines get caught by this event handler.I think we should be able to rely on SQLAlchemy's reset on return behaviour to rollback transactions when they're reset rather than call rollback ourselves. That behaviour is also something end users can configure, so it'd be good to respect their settings if they disable the behaviour.
Fixes: #706