Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use read only transaction more on search #1726

Merged
merged 7 commits into from
Jan 25, 2024
Merged

Conversation

lferran
Copy link
Contributor

@lferran lferran commented Jan 15, 2024

Description

Even if we had the shared transaction logic in place, many code in the search endpoints was creating extra transactions.
This caused a drop in performance specially when using the PG driver for the maindb (Hybrid on-prem deployments!)

The improvement on the performance tests is around 2x:

On main

Metrics summary:
- find_latency_s -> p50: 153.69ms p95: 283.02ms
- search_latency_s -> p50: 873.75ms p95: 1.19s
- suggest_latency_s -> p50: 249.35ms p95: 496.69ms
**** Molotov v2.6. Happy breaking! ****
SUCCESSES: 652 | FAILURES: 0
*** Bye ***

On the current branch

Metrics summary:
- find_latency_s -> p50: 58.67ms p95: 106.33ms
- search_latency_s -> p50: 443.13ms p95: 562.02ms
- suggest_latency_s -> p50: 118.93ms p95: 224.92ms
**** Molotov v2.6. Happy breaking! ****
SUCCESSES: 1276 | FAILURES: 0
*** Bye ***

How was this PR tested?

Describe how you tested this PR.

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (65c7d4f) 82.16% compared to head (ea83a68) 82.19%.
Report is 1 commits behind head on main.

Files Patch % Lines
nucliadb/nucliadb/common/datamanagers/kb.py 72.41% 8 Missing ⚠️
nucliadb/nucliadb/common/maindb/pg.py 0.00% 3 Missing ⚠️
nucliadb/nucliadb/search/predict.py 50.00% 2 Missing ⚠️
nucliadb/nucliadb/common/datamanagers/cluster.py 90.90% 1 Missing ⚠️
nucliadb/nucliadb/ingest/orm/knowledgebox.py 50.00% 1 Missing ⚠️
nucliadb_utils/nucliadb_utils/keys.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1726      +/-   ##
==========================================
+ Coverage   82.16%   82.19%   +0.02%     
==========================================
  Files         335      336       +1     
  Lines       19803    19847      +44     
==========================================
+ Hits        16271    16313      +42     
- Misses       3532     3534       +2     
Flag Coverage Δ
ingest 69.04% <50.00%> (-0.02%) ⬇️
node-sidecar 95.39% <ø> (ø)
nucliadb 70.34% <76.56%> (-0.06%) ⬇️
reader 79.82% <ø> (ø)
sdk 40.64% <ø> (ø)
standalone 88.29% <ø> (ø)
train 63.60% <ø> (ø)
utils 81.62% <0.00%> (+0.19%) ⬆️
writer 85.11% <ø> (+0.68%) ⬆️

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.

@lferran
Copy link
Contributor Author

lferran commented Jan 15, 2024

Related ticket [sc-8127]

Copy link

This pull request has been linked to Shortcut Story #8127: refactor transaction context var on search.

@lferran lferran requested a review from a team January 16, 2024 15:40
@lferran lferran force-pushed the use-readonly-txn-more branch 2 times, most recently from aa3a399 to 6928e0e Compare January 16, 2024 15:48
@lferran lferran force-pushed the use-readonly-txn-more branch 2 times, most recently from 42d7d52 to 98fbb28 Compare January 19, 2024 08:41
@lferran lferran merged commit 3ef62fb into main Jan 25, 2024
92 of 93 checks passed
@lferran lferran deleted the use-readonly-txn-more branch January 25, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants