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

🐛 Reduce DB connection locking in storage #4984

Merged

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Nov 6, 2023

What do these changes do?

It is not necessary to expand directories (access S3) while holding a transaction in Postgres.

This will free up more connections.

Related issue/s

How to test

DevOps Checklist

@GitHK GitHK self-assigned this Nov 6, 2023
@GitHK GitHK added a:storage issue related to storage service changelog:🐛bugfix labels Nov 6, 2023
@GitHK GitHK added this to the 7peaks milestone Nov 6, 2023
@GitHK GitHK marked this pull request as ready for review November 6, 2023 10:58
@GitHK GitHK requested a review from sanderegg as a code owner November 6, 2023 10:58
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #4984 (27e1400) into master (420f177) will decrease coverage by 20.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4984      +/-   ##
=========================================
- Coverage    85.4%   65.4%   -20.1%     
=========================================
  Files        1239     554     -685     
  Lines       51029   28309   -22720     
  Branches     1080     195     -885     
=========================================
- Hits        43612   18516   -25096     
- Misses       7180    9744    +2564     
+ Partials      237      49     -188     
Flag Coverage Δ
integrationtests 63.5% <ø> (-1.4%) ⬇️
unittests 90.7% <100.0%> (+5.9%) ⬆️

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

Files Coverage Δ
...rage/src/simcore_service_storage/simcore_s3_dsm.py 94.3% <100.0%> (+<0.1%) ⬆️

... and 981 files with indirect coverage changes

@GitHK GitHK changed the title 🐛 Reduce DB connection locking when in storage 🐛 Reduce DB connection locking in storage Nov 6, 2023
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

I am not sure I understand how you fixed the issue here. I would like to better understand that and I have some proposals for you to change.

@GitHK
Copy link
Contributor Author

GitHK commented Nov 7, 2023

I am not sure I understand how you fixed the issue here. I would like to better understand that and I have some proposals for you to change.

@sanderegg

There is an open connection/transaction to the database while making calls to S3 to expand the directory (listing all files inside). It is not required to list the contents of the directory while holding the transaction opened.

@GitHK GitHK requested review from pcrespov and sanderegg November 7, 2023 10:56
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

still have questions, and want to discuss

@GitHK GitHK requested a review from sanderegg November 7, 2023 13:15
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thank you very much for the additional changes!

@GitHK GitHK enabled auto-merge (squash) November 7, 2023 15:03
Copy link

sonarqubecloud bot commented Nov 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

Copy link

codeclimate bot commented Nov 7, 2023

Code Climate has analyzed commit 27e1400 and detected 0 issues on this pull request.

View more on Code Climate.

@GitHK GitHK merged commit 5ebdd6e into ITISFoundation:master Nov 7, 2023
@GitHK GitHK deleted the pr-osparc-more-aggressive-conection-release branch November 8, 2023 06:49
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Nov 23, 2023
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:storage issue related to storage service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants