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.

Sign up for GitHub

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

Jump to bottom

editoast: database connection pool tools #7281

Merged
merged 7 commits into from
May 23, 2024
Merged

editoast: database connection pool tools #7281

merged 7 commits into from
May 23, 2024

Conversation

Wadjetz
Copy link
Member

@Wadjetz Wadjetz commented Apr 19, 2024 •
edited
Loading

part of #6980

  • Creation of custom ConnectionPool
  • Isolation of tests with test_connection transaction (that do not commit the transaction)
  • Exemple of usage in documents api
  • Test App Builder

@Wadjetz Wadjetz added area:editoast Work on Editoast Service kind:refacto-task Task related to Refactorization Epic labels Apr 19, 2024
@Wadjetz Wadjetz self-assigned this Apr 19, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.20690% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 29.31%. Comparing base (fd6a65c) to head (3997ded).

Files Patch % Lines
editoast/src/modelsv2/database/connection_pool.rs 80.82% 14 Missing ⚠️
editoast/src/views/test_app.rs 89.88% 9 Missing ⚠️
editoast/src/modelsv2/database/connection_error.rs 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@             Coverage Diff              @@
##                dev    #7281      +/-   ##
============================================
+ Coverage     29.26%   29.31%   +0.05%     
  Complexity     2012     2012              
============================================
  Files          1194     1196       +2     
  Lines        146864   146983     +119     
  Branches       2889     2889              
============================================
+ Hits          42978    43088     +110     
- Misses       102186   102195       +9     
  Partials       1700     1700
Flag Coverage Δ
core 75.07% <ø> (ø)
editoast 72.47% <86.20%> (+0.08%) ⬆️
front 9.75% <ø> (ø)
gateway 2.41% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 84.23% <ø> (ø)

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.

@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 3 times, most recently from 1c74518 to 112e4a5 Compare April 19, 2024 11:34
@Wadjetz Wadjetz changed the title editoast: connection pool editoast: database connection pool tools Apr 19, 2024
@Wadjetz Wadjetz requested a review from leovalais April 19, 2024 11:37
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch from 112e4a5 to 7a786ca Compare April 19, 2024 13:13
leovalais
leovalais reviewed Apr 19, 2024
View reviewed changes
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

Thanks for starting on that issue it's really not an easy task.
I have a few comments, mostly API-related.

editoast/src/connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/mod.rs Outdated Show resolved Hide resolved
editoast/src/views/documents.rs Outdated Show resolved Hide resolved
editoast/src/views/mod.rs Outdated Show resolved Hide resolved
editoast/src/views/mod.rs Outdated Show resolved Hide resolved
editoast/src/views/documents.rs Show resolved Hide resolved
editoast/src/connection_pool.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 7 times, most recently from 43cf7df to d33d06d Compare April 26, 2024 07:50
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch from d33d06d to bcd3d7c Compare April 29, 2024 15:22
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 2 times, most recently from 9b719b5 to 9818c2b Compare May 14, 2024 08:43
woshilapin
woshilapin reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

It'd be nice to have all of this new setup.

I've made a few comments because I believe we might not need so many new functions, specific for tests... Let's discuss it, there might be some constraints that I'm not aware of.

editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/views/mod.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 5 times, most recently from ad35c35 to 2843c2c Compare May 14, 2024 14:38
@Wadjetz Wadjetz marked this pull request as ready for review May 14, 2024 14:39
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch from a46a2f0 to 5fde94d Compare May 16, 2024 13:42
leovalais
leovalais reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

We discussed these points orally, just leaving them here for traceability.

editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/connection_pool/db_connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/views/documents.rs Outdated Show resolved Hide resolved
editoast/src/views/documents.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 4 times, most recently from 7206586 to 475dc19 Compare May 17, 2024 13:45
@Wadjetz Wadjetz requested a review from leovalais May 17, 2024 13:46
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 2 times, most recently from cad1b50 to 77f1357 Compare May 21, 2024 07:23
leovalais
leovalais approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

Approving in order not to block this PR, but since I'm participating now I'd like another review by @woshilapin plz 🙏

@leovalais leovalais requested a review from woshilapin May 21, 2024 08:47
woshilapin
woshilapin reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

It seems the complexity of the PR increased significantly since last time I reviewed it. I don't really understand why yet. I've made a bunch of comments, maybe not all make sense. We might need to discuss it.

editoast/src/modelsv2/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/src/modelsv2/database/connection_pool.rs Show resolved Hide resolved
editoast/src/views/test_app.rs Outdated Show resolved Hide resolved
editoast/src/views/test_app.rs Outdated Show resolved Hide resolved
editoast/src/views/test_app.rs Show resolved Hide resolved
editoast/src/views/test_app.rs Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch 3 times, most recently from b8c077b to fb57711 Compare May 22, 2024 13:38
woshilapin
woshilapin reviewed May 23, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

I resolved some of the comments and left the one I believe were either not clear, or not addressed. Tell me if you need any help, or if any comment is not understandable.

@Wadjetz Wadjetz force-pushed the ebe/editoast-connection-pool branch from 2c0c392 to 3997ded Compare May 23, 2024 12:45
@Wadjetz Wadjetz requested a review from woshilapin May 23, 2024 13:22
@Wadjetz Wadjetz added this pull request to the merge queue May 23, 2024
Merged via the queue into dev with commit 9ba26e9 May 23, 2024
17 checks passed
@Wadjetz Wadjetz deleted the ebe/editoast-connection-pool branch May 23, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woshilapin woshilapin woshilapin approved these changes

@leovalais leovalais leovalais approved these changes

@hamz2a hamz2a hamz2a approved these changes

Assignees

@Wadjetz Wadjetz

Labels
area:editoast Work on Editoast Service kind:refacto-task Task related to Refactorization Epic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Wadjetz @codecov-commenter @woshilapin @leovalais @hamz2a