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

Fix pool usage across the indexer #403

Merged
merged 6 commits into from
Dec 6, 2022
Merged

Fix pool usage across the indexer #403

merged 6 commits into from
Dec 6, 2022

Conversation

deekerno
Copy link
Contributor

@deekerno deekerno commented Dec 6, 2022 •
edited
Loading

Note: This does not fix SQLite; this is just a cleanup PR while I continue to fix the issue.

Changelog

  • Consolidates pool usage across application; we were creating multiple pools across the indexer, which meant that max_connections() and other settings weren't being respected
  • Adds pool and connection options; we're now able to change pool and connection settings (e.g. log level), if so desired
  • Changes some unwrap()s to expect()s

Testing Plan

CI should pass. Starting the service using Postgres should also work as expected.

@deekerno deekerno added bug Something isn't working code quality labels Dec 6, 2022
@deekerno deekerno self-assigned this Dec 6, 2022
@deekerno deekerno mentioned this pull request Dec 6, 2022
Closed
@deekerno
Allow for both pool and connection options
1f3efd0
@deekerno
Consolidate pools across application
760e506 
While trying to fix the SQLite breakage, I noticed that the
max_connections() setting was not being respected. After some
investigation, I realized that we create multiple pools across
the application instead of using one pool and acquiring the
connections from there. This commit fixes that and brings
the application more in line with the use case of a database pool.
@deekerno
Change some unwraps to expects
e078796
@deekerno
Clippy fixes
ed69d2b
@deekerno deekerno force-pushed the deekerno/369-sqlite-why-hast-thou-forsaken-us branch from e730ac6 to ed69d2b Compare December 6, 2022 16:19
@deekerno deekerno marked this pull request as ready for review December 6, 2022 17:18
@deekerno deekerno requested a review from ra0x3 December 6, 2022 17:18
ra0x3
ra0x3 suggested changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@ra0x3 ra0x3 left a comment

Choose a reason for hiding this comment

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

  • Looks fine.
  • Some minor code quality comments

packages/fuel-indexer-api-server/src/api.rs Show resolved Hide resolved
packages/fuel-indexer-api-server/src/bin/fuel-indexer-api-server.rs Outdated Show resolved Hide resolved
packages/fuel-indexer-database/src/lib.rs Outdated Show resolved Hide resolved
packages/fuel-indexer/src/bin/fuel-indexer.rs Outdated Show resolved Hide resolved
@deekerno deekerno enabled auto-merge (squash) December 6, 2022 19:10
@deekerno deekerno requested a review from ra0x3 December 6, 2022 19:10
ra0x3
ra0x3 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@ra0x3 ra0x3 left a comment

Choose a reason for hiding this comment

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

✅

@deekerno deekerno merged commit b474f1d into master Dec 6, 2022
@deekerno deekerno deleted the deekerno/369-sqlite-why-hast-thou-forsaken-us branch December 6, 2022 20:02
ra0x3 pushed a commit that referenced this pull request Dec 15, 2022
@deekerno
Fix pool usage across the indexer (#403)
d68de91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ra0x3 ra0x3 ra0x3 approved these changes

Assignees

@deekerno deekerno

Labels
bug Something isn't working
Projects
Fuel Network
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@deekerno @ra0x3