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

Do not reinit db connection locally #3716

Merged
merged 13 commits into from
Nov 17, 2023
Merged

Do not reinit db connection locally #3716

merged 13 commits into from
Nov 17, 2023

Conversation

flozia
Copy link
Collaborator

@flozia flozia commented Nov 9, 2023

Description

When developing locally we reinitialize and create a new database connection each time the server reloads. When hitting your local db client threshold this results in an Unhandled Runtime Error you have likely already had the pleasure to enjoy instead of your latest code changes.

For local development we can just store the initialized db in global and hopefully you’ll have to npm run dev a lot less now.

image

How to test

Try developing and making changes for a bit on/from this branch. If you are not running into the Error: sorry, too many clients already view or any new issues related to your local database connection all should be good. :)

@flozia flozia requested review from Vinnl, rhelmer, mansaj and codemist and removed request for Vinnl and rhelmer November 9, 2023 18:37
@flozia flozia marked this pull request as draft November 9, 2023 18:43
@flozia flozia marked this pull request as ready for review November 9, 2023 18:50
@pdehaan
Copy link
Contributor

pdehaan commented Nov 9, 2023

Not sure any of these are relevant to the refactor, but possibly worth verifying:

git grep -n "/knexfile.js" src | cat

src/db/index.js:6:import knexConfig from './knexfile.js'
src/scripts/convertBreachResolutions.js:12:import knexConfig from "../db/knexfile.js";
src/scripts/kube-jobs/convertBreachResolutions.js:12:import knexConfig from "../../db/knexfile.js";
src/scripts/kube-jobs/recencyToBreachId.js:14:import knexConfig from "../../db/knexfile.js";
src/scripts/migrationCleanup.js:11:import knexConfig from "../db/knexfile.js";
src/scripts/prod-experiments/benchmarkNoUpsert.js:12:import knexConfig from "../../db/knexfile.js";
src/scripts/prod-experiments/benchmarkWithUpsert.js:12:import knexConfig from "../../db/knexfile.js";
src/scripts/prod-experiments/pullExperiment100.js:11:import knexConfig from "../../db/knexfile.js";
src/scripts/prod-experiments/pullExperiment1000.js:11:import knexConfig from "../../db/knexfile.js";

Looks like the overwhelming majority of the remaining usages are in src/scripts/**.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is also used in our cron jobs and queue scripts, which don't support TypeScript - so it'll probably have to stay .js for now.

(Problems like those are currently non-obvious, which is why I think separate projects with their own specific setup are probably safer.)

Copy link
Collaborator Author

@flozia flozia Nov 10, 2023

Choose a reason for hiding this comment

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

Because of that reason I did not make any changes to the scripts that @pdehaan grepped above and only to the relevant files for running the application. But we can also just have this a .js file and replace it everywhere. I was just hesitant to do so because we will likely only notice if anything goes wrong until it is deployed.

Edit: Ok, the changes are still being used in some scripts that import this file. I’ll make this a .js file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I figured those scripts possibly import the DB modules we have? But Joe and Rob will know better.

Copy link
Collaborator Author

@flozia flozia Nov 10, 2023

Choose a reason for hiding this comment

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

Yup! … and to @pdehaan’s note:

Not sure any of these are relevant to the refactor, but possibly worth verifying:
[…]
Looks like the overwhelming majority of the remaining usages are in src/scripts/**.

Now I can drop this in everywhere: 5d719fa.

Copy link
Collaborator Author

@flozia flozia Nov 17, 2023

Choose a reason for hiding this comment

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

@mansaj Do you have any concerns with the db and script related changes?

@flozia flozia force-pushed the too-many-db-connections branch from 9cd3f41 to 5d719fa Compare November 10, 2023 12:56
@flozia flozia self-assigned this Nov 10, 2023
Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

I've been trying this out locally today and haven't noticed the problem with DB connections running out.

@flozia flozia merged commit fbd4502 into main Nov 17, 2023
10 checks passed
@flozia flozia deleted the too-many-db-connections branch November 17, 2023 22:55
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.

4 participants