Optimize update_commit_from_provider_info
#855
Conversation
This makes the `possibly_` variant of this function sync, to avoid an unnecessary `async_to_sync` call. The main function is changed in such a way that: - It early-returns when no commit was found, de-indenting the rest of the block - Most importantly, it moves some of the field writes around, and adds an explicit `flush` at the end. That way, it avoids splitting the `UPDATE` into two different queries, and rather does one update at the end. - A query indirection doing a `COUNT` before the actual query is removed by just doing the filtering on the Python side - It might avoid a second provider request for a bitbucket edgecase.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 444 445 +1
Lines 36057 36102 +45
==========================================
+ Hits 35346 35386 +40
- Misses 711 716 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #855 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 444 445 +1
Lines 36057 36102 +45
==========================================
+ Hits 35346 35386 +40
- Misses 711 716 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #855 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 444 445 +1
Lines 36057 36102 +45
==========================================
+ Hits 35346 35386 +40
- Misses 711 716 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #855 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 444 445 +1
Lines 36057 36102 +45
==========================================
+ Hits 35346 35386 +40
- Misses 711 716 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| pull_details = await repository_service.get_pull_request(pullid) | ||
| commit.branch = pull_details["base"]["branch"] | ||
|
|
||
| db_session.flush() |
There was a problem hiding this comment.
Just for my owner understanding, I'm curious to how this flush helps with reducing the number of updates. I had assumed that we had to either flush (or commit, which does a flush first) for the update to "happen" in the transaction, so I might have the wrong mental model on how it works!
There was a problem hiding this comment.
The number of updates are reduced by moving code around.
Previously, some fields on the current Commit were written, and those were apparently implicitly flushed by sqalchemy as another function was querying Commits.
Now the queries happen first, and all the field updates second, whereby the UPDATE should update all the fields at once.
The flush is just there to force the UPDATE to happen inside of this function, as previously, it would be delayed till later on nested in a completely different call chain.
Though to be quite honest, I haven’t verified this behavior locally. We will see this more clearly in tracing.
This makes the
possibly_variant of this function sync, to avoid an unnecessaryasync_to_synccall.The main function is changed in such a way that:
flushat the end. That way, it avoids splitting theUPDATEinto two different queries, and rather does one update at the end.COUNTbefore the actual query is removed by just doing the filtering on the Python sidePart of codecov/engineering-team#2824