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

chore: add max connections to warehouse and pgnotifier #3597

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Jul 7, 2023

Description

  • Setting maxOpenConnections for Warehouse and PgNotifier db handle.

Notion Ticket

https://www.notion.so/rudderstacks/Max-open-connection-for-warehouse-and-postgres-91e4987ff26b41f6a7aa337fa3cb8f80?pvs=4

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (abd9c8c) 68.01% compared to head (40587e8) 68.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3597      +/-   ##
==========================================
- Coverage   68.01%   68.00%   -0.01%     
==========================================
  Files         318      318              
  Lines       50375    50380       +5     
==========================================
+ Hits        34262    34263       +1     
- Misses      13880    13883       +3     
- Partials     2233     2234       +1     
Impacted Files Coverage Δ
services/pgnotifier/pgnotifier.go 71.13% <100.00%> (+0.16%) ⬆️
warehouse/warehouse.go 64.69% <100.00%> (+0.05%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@fracasula fracasula left a comment

Choose a reason for hiding this comment

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

Looks good, if needed moving forward we can differentiate between master and slave but for now I don't see a reason why. Let me know if you disagree of course.

@achettyiitr
Copy link
Member Author

achettyiitr commented Jul 7, 2023

Looks good, if needed moving forward we can differentiate between master and slave but for now I don't see a reason why. Let me know if you disagree of course.

I am not sure how much benefit it can provide. If we need separate configs for master and slave we can always set them as ENVs since both of these have separate deployments.
warehouse-master-deployment
warehouse-slave-deployment

@achettyiitr achettyiitr merged commit f632a53 into master Jul 11, 2023
72 of 73 checks passed
@achettyiitr achettyiitr deleted the chore.max-connections branch July 11, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants