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

Db connection pooling dev #5662

Draft
wants to merge 18 commits into
base: develop
Choose a base branch
from
Draft

Db connection pooling dev #5662

wants to merge 18 commits into from

Conversation

MSDrao
Copy link
Contributor

@MSDrao MSDrao commented Dec 2, 2024

Pull Request Checklist:

  • Positive Test Case Written by Dev
  • Automated Testing
  • Sufficient User and Developer Documentation
  • Passing Jenkins Build
  • Peer Code review and approval

Positive Test Case

  1. [Enter positive test case here]

@MSDrao MSDrao linked an issue Dec 2, 2024 that may be closed by this pull request
@martinseul martinseul requested a review from devincowan December 2, 2024 16:24
Copy link

github-actions bot commented Dec 2, 2024

Test Results

    2 files  ±0      2 suites  ±0   1h 34m 18s ⏱️ + 2m 26s
1 415 tests ±0  1 399 ✅ ±0  16 💤 ±0  0 ❌ ±0 
1 527 runs  ±0  1 509 ✅ ±0  18 💤 ±0  0 ❌ ±0 

Results for commit a858959. ± Comparison against base commit 69f2ddc.

♻️ This comment has been updated with latest results.

@devincowan
Copy link
Contributor

@MSDrao looks like you have some broken tests. I will review once those are resolved.

@devincowan
Copy link
Contributor

Also noting that this work will take some tooling to get pgbouncer running in k8s

@MSDrao MSDrao marked this pull request as draft December 16, 2024 05:25
@MSDrao MSDrao self-assigned this Dec 16, 2024
@@ -43,6 +43,26 @@ services:
- "54322:5432"
stdin_open: true
tty: true
pgbouncer:
image: edoburu/pgbouncer:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local-dev.yml Show resolved Hide resolved
local-dev.yml Outdated Show resolved Hide resolved
@MSDrao MSDrao requested a review from devincowan December 25, 2024 16:25
Copy link
Contributor

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good!
next step is some load testing...

@devincowan
Copy link
Contributor

@MSDrao I committed your locustfile.py here:
0469853
and bumping MAX_CLIENT_CONNECTIONS to 1000 seemed to work for me:
Out of ~1600 hits to /user I had 0% failure
image

@devincowan devincowan force-pushed the db-connection-pooling-dev branch from 12781d1 to 0469853 Compare January 9, 2025 16:55
@devincowan
Copy link
Contributor

devincowan commented Jan 13, 2025

Posting some initial load tests here...not looking promising at this point but I'm going to keep at it.

  1. Run without pgbouncer: https://github.com/devincowan/hydroshare/actions/runs/12751630824
  2. With pgbouncer: https://github.com/devincowan/hydroshare/actions/runs/12751427298 (max connections 1000)
  3. W/ pgbouncer: https://github.com/devincowan/hydroshare/actions/runs/12752226259 (max conn 300)

Hope to show something more promising in a bit...

UPDATE with some additional tests:

  1. With pgbouncer: https://github.com/devincowan/hydroshare/actions/runs/12753100485
    w-pg-bounce-dcowan-locust-results.zip
    with-pgbouncer-total_requests_per_second_1736791312 155

  2. Without pgbouncer: https://github.com/devincowan/hydroshare/actions/runs/12753260321
    no-pg-bounce-dcowan-locust-results(1).zip
    no-pgbouncer-total_requests_per_second_1736791897 481

There isn't a huge benefit here, but it does look like there is some improvement (pgbouncer test allows higher # of users before failure)

@devincowan
Copy link
Contributor

ahhh here we go!
Adding more settings: https://github.com/CUAHSI/platform-recipes/pull/71/commits/b5463ea8e0a870358df935dbe198270b7b01ab83
And then re-running the tests:
https://github.com/devincowan/hydroshare/actions/runs/12753590880
with-pg-and-settings-locust-results(11).zip
with-pg-and-updated-settings-total_requests_per_second_1736792988 424

☝️ we can get to much higher numbers of users before getting 502s and we can also achieve significantly higher RPS!

Copy link
Contributor

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after load testing on dev-1 I think this looks good!

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.

consider connection pooling for better db performance
2 participants