fix(key-value): use flush instead of commit

[villebro]

SUMMARY

In the key value commands, we are often returning the keys from newly generated ORM objects, which are persisted in the key_value table. As we're currently using commit() to persist the objects prior to reading the keys, the session is reset, requiring a new session for fetching the keys. This can cause issues when distributing metastore reads to read replicas, as the data may sometimes not be available, causing an ObjectDeletedError exception during CreateKeyValueCommand.run():

sqlalchemy.orm.exc.ObjectDeletedError: Instance '<KeyValueEntry at 0x7fe508293280>' has been deleted, or its row is otherwise not present.

A simple fix is to use flush(), which will ensure the same session is used, as stated in SIP-99A:

It is worth noting that objects within the session, in either pending* or persisted state, can be queried albeit not having been committed to the database. We have a tendency to over commit (be that in the code or tests) resulting in fractured/partial atomic units. Typically working with objects in a persisted state is sufficient.

As we're now proposing to replace commit() with flush() in the key value commands, the session will need to be explicitly committed once the full unit of work is done. For example, for the create/upsert commands, we will now only commit after the last task finishes, ensuring atomicity:

1. delete expired entries
2. insert new entry
3. commit

However, there are instances where we explicitly need commits to happen during a single request lifecycle. For instance, the Key Value Distributed Lock needs to commit the lock to the metastore so other workers can observe it for the duration of the lock event. The same applies to the Metastore cache, where every set/add/delete operation should commit to the cache. For this reason we're adding explicit commit() calls both in the Metastore Cache and Lock context manager to ensure the values are persisted in the db during execution flow.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.73%. Comparing base (76d897e) to head (01333c7).
Report is 1094 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29286       +/-   ##
===========================================
+ Coverage   60.48%   83.73%   +23.24%     
===========================================
  Files        1931      518     -1413     
  Lines       76236    37566    -38670     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31456    -14658     
+ Misses      28017     6110    -21907     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.93% <20.00%> (-0.23%)	⬇️
javascript	?
mysql	77.23% <90.00%> (?)
postgres	77.34% <90.00%> (?)
presto	53.54% <20.00%> (-0.26%)	⬇️
python	83.73% <100.00%> (+20.24%)	⬆️
sqlite	76.79% <90.00%> (?)
unit	59.25% <50.00%> (+1.62%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[john-bodley]

Does the UpsertKeyValueCommand not commit? Note that hopefully (if merged) that #24969 should remove the need to have to commit in various places given it violates the "unit of work" philosophy.

[villebro]

Here I'm proposing to not commit in these commands, as we have cases where we want to chain multiple commands together. But if we consider each Key Value command a unit of work, then we should naturally commit there.

[nytai]

i think there's a case to be made for both. Maybe we can have default behavior be to commit but provide an option to skip for use cases where there's a command chaining multiple sub-commands? It would be inconsistent for some commands to commit while others requiring the caller to commit.

[villebro]

While this has previously been considered an antipattern (=having commit: bool flag in these types of methods or similar), I'm personally also kind of leaning in that direction. This would make it possible to use all existing commands both as full units of work, or as part of a bigger chain, with minimal code duplication. Thoughts @john-bodley ? Also pinging @michael-s-molina as you've worked on these components in the past.

[john-bodley]

SIP-99B will handle the chaining of commands (if necessary) via nested sessions where only the outermost session commits.

I think this logic is fine for now, though @villebro some of it maybe updated if/when I get my SIP-99B through.

[michael-s-molina]

This would make it possible to use all existing commands both as full units of work, or as part of a bigger chain, with minimal code duplication

We'll achieve that with nested transactions using begin_nested. Check #24969 for reference.

[villebro]

We'll achieve that with nested transactions using being_nested. Check #24969 for reference.

@michael-s-molina I read up on the docs I could find, and I agree, this should be an elegant solution to this dual use case.

@john-bodley it would be great if you could add a description to #24969 so we can start reviewing/testing it. I'm keen on getting this important refactor in, as it will have a profound impact on the general quality and perormance of the backend.