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

Avoid having multiple catalog connection pools #793

Merged
merged 7 commits into from
Dec 12, 2023
Merged

Avoid having multiple catalog connection pools #793

merged 7 commits into from
Dec 12, 2023

Conversation

serprex
Copy link
Contributor

@serprex serprex commented Dec 11, 2023

Connection pools are meant to be shared, so remove pool from monitoring

Also go one step further: cache connection pool from env.go into global This requires never closing pool returned by GetCatalogConnectionPoolFromEnv

Run migrations for CI because monitoring logic is now non optional

Opens up reusing pool for #778

@serprex
Copy link
Contributor Author

serprex commented Dec 11, 2023

Storing pool in global variable might be going too far, open to rolling that change back

flow/e2e/postgres/peer_flow_pg_test.go Outdated Show resolved Hide resolved
flow/e2e/postgres/peer_flow_pg_test.go Outdated Show resolved Hide resolved
flow/e2e/postgres/peer_flow_pg_test.go Outdated Show resolved Hide resolved
flow/e2e/s3/cdc_s3_test.go Outdated Show resolved Hide resolved
flow/e2e/s3/cdc_s3_test.go Outdated Show resolved Hide resolved
flow/e2e/s3/qrep_flow_s3_test.go Outdated Show resolved Hide resolved
flow/e2e/sqlserver/qrep_flow_sqlserver_test.go Outdated Show resolved Hide resolved
@serprex serprex force-pushed the one-pool branch 2 times, most recently from 89ff454 to 78ecd09 Compare December 11, 2023 18:17
flow/e2e/postgres/qrep_flow_pg_test.go Outdated Show resolved Hide resolved
flow/e2e/s3/qrep_flow_s3_test.go Outdated Show resolved Hide resolved
@serprex serprex force-pushed the one-pool branch 3 times, most recently from 2ee510b to dc0feff Compare December 11, 2023 19:40
@iskakaushik
Copy link
Contributor

In postgres.go we also have a replPool and another pool IIRC. I don't think those need to be pools as well. We can refactor them later though.

Connection pools are meant to be shared, so remove pool from monitoring

Also go one step further: cache connection pool from env.go into global
This requires never closing pool returned by GetCatalogConnectionPoolFromEnv
@serprex serprex force-pushed the one-pool branch 4 times, most recently from d4d1963 to aac676a Compare December 11, 2023 21:52
@serprex serprex enabled auto-merge (squash) December 12, 2023 00:32
@serprex serprex disabled auto-merge December 12, 2023 01:22
@serprex serprex enabled auto-merge (squash) December 12, 2023 01:57
@serprex serprex merged commit de903b9 into main Dec 12, 2023
12 checks passed
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.

3 participants