Gracefully migrate transactions #262
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
dogversioning
force-pushed
the
mg/migrate_transactions
branch
from
7795dab to
e4cb339
Compare
| clear=True, | ||
| ) | ||
| @mock.patch("pyathena.connect") | ||
| def test_migrate_transactions_athena(mock_pyathena): |
There was a problem hiding this comment.
this test is extremely silly.
cumulus_library/log_utils.py
Outdated
| except Exception as e: | ||
| # Migrating logging tables | ||
| if "lib_transactions" in table_name: |
There was a problem hiding this comment.
There's not a tighter exception to grab because of two databases, I assume? I don't suppose this one happens to already fall under the operational_errors() set?
But also, stepping back - this is a pretty awkward code path 😄 -- one that you hadn't even planned to write until I hit this and was grumbling about the UX of it -- if you think this is too gross but you were writing this anyway for my sake, I'd be fine with something as basic as "detect this case, print a nice error message about what to do next" if you didn't want to carry this kind of alter logic going forward.
There was a problem hiding this comment.
i didn't think of operational_errors - and it might be worth keeping this around if only to actually have something that is using them, since right now they're orphans - the duckdb one is duckdb.duckdb.BinderException, which is pretty broad, but we could add it to this list?
anyway, it's here now, and realistically it's probably going to happen again, so we might as well just ride with 'ok this is where DB migrations go' - this way will preserve state at least, which is different than just dropping, so it is probably 'correct' even if the edge cases are marginal.
There was a problem hiding this comment.
since right now they're orphans
sql_utils.py uses operational_errors() I think.
anyway, it's here now
Fair - just we might want to give ourselves the grace to chuck it down the line as "deprecated migration stuff" if we ever add more database backends etc.
There was a problem hiding this comment.
sql_utils.py uses operational_errors() I think.
so it does - i don't think that's covered under test yet, which is why that's all #pragma'd up in databases.
dogversioning
force-pushed
the
mg/migrate_transactions
branch
from
c53cb2b to
2abfe1a
Compare
This PR addresses #259 by altering a given lib_transaction table if an error is raised.
Also added manifest/config to the external API.
Checklist
docs/) needs to be updated