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

Optional standalone dbconnector #204

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

ryanartecona
Copy link
Contributor

Allow dbconnector to run as a standalone deployment for scale & resource flexibility for large deployments.

@@ -0,0 +1,2 @@
dbconnector:
Copy link
Contributor

Choose a reason for hiding this comment

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

glad to see this feature of testing with random extra charts was useful lol

# cpu: 2048m
# memory: 4096Mi

config:
Copy link
Contributor

@jjlgao jjlgao Jan 16, 2025

Choose a reason for hiding this comment

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

did you mean to set this config intentionally? I'm surprised to see such a high default postgres max pool size, especially on the dbc deployment, as that would be specific to the Postgres connector which isn't global (also I believe the Postgres connector uses a different env var).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

putting the config here was an intentional refactor from the internal fork I used as reference, yes. but the default values I don't have strong opinions about, these are mostly guesses. what do you think they should be?

I did confirm this templates out to the correct env var (DBCONNECTOR_POSTGRES_POOL_MAX_SIZE), which defaults to 10 in our code if unspecified. should I just use that?

- name: DB_CONNECTOR_HOST
value: http://{{ template "retool.fullname" . }}-dbconnector
- name: DB_CONNECTOR_PORT
value: '3002'
Copy link
Contributor

@jjlgao jjlgao Jan 16, 2025

Choose a reason for hiding this comment

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

nit: would consider making this configurable with a default of 3002, unless we hardcode the backend port as well (I think that should also be configurable, but if it's already there then we can just make that improvement separately).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! I couldn't do it exactly how backend does it, but it's at least configurable.

ports:
- protocol: TCP
name: http-server
port: 3002
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: similarly to the env vars, would make this configurable if possible

- name: SERVICE_TYPE
value: {{ join "," $serviceType }}
{{- if $.Values.dbconnector.enabled }}
- name: DB_CONNECTOR_HOST
value: http://{{ template "retool.fullname" . }}-dbconnector
Copy link
Contributor

@jjlgao jjlgao Jan 16, 2025

Choose a reason for hiding this comment

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

this would point workflows-backend at the generic dbc deployment -- is it intentional that we aren't creating a workflows-dbconnector deployment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's intentional, yes, but not unchangeable. I think splitting dbconnector out of main backend is likely the main scalability unlock for large single-tenant deployments, and we're only just now adding it to retool-helm. going even further and splitting out a workflows-dbconnector from the main dbconnector seems premature, given that workflows doesn't need its own dbconnector to function and we do that mainly for scale reasons in our multi-tenant cloud.

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.

2 participants