-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(condo): INFRA-202 multiple database sources #4618
base: master
Are you sure you want to change the base?
Conversation
sitozzz
commented
Apr 15, 2024
•
edited
edited
- Need to deal with adapter cache!!!
- Stand tests
- Load tests
6a93b34
to
db441c8
Compare
1f957e4
to
046aaa6
Compare
run: | | ||
docker run -e POSTGRES_USER=$POSTGRES_USER -e POSTGRES_PASSWORD=$POSTGRES_PASSWORD -e POSTGRES_DB=$POSTGRES_DB -p="127.0.0.1:5432:5432" -d swr.ru-moscow-1.hc.sbercloud.ru/doma/utils/postgres:13.2 | ||
touch ./pg_hba.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script looks very complex. Ideally, I'd like to see something like docker-compose up postgres postgres-replica
, and all the magic with replication should happen automatically. This way, there's no need to do it all manually in CI, and it can be properly tested locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's true. But for now we can't run compose modules with self-hosted runner. This will be fixed in the future updates
db580d7
to
04db392
Compare
04db392
to
87a36fd
Compare
cefc8ee
to
c452413
Compare
…ections handling option [SQUASH THIS COMMIT DUE TO TEST CHANGES]
…using ScalableDatabaseAdapter
…l mapping function due to api changes
08e8972
to
3c91f98
Compare
Quality Gate passedIssues Measures |
const checkUseMasterSingle = (object) => { | ||
// if object.method equals "insert", "del" or "update" then use master endpoint | ||
if (object.method !== undefined) { | ||
return ['insert', 'del', 'update'].includes(object.method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about something like alter
?
return ['create'].some(sub => object.sql.includes(sub)) | ||
} | ||
|
||
const checkUseMasterMultiple = (array) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array.some(checkUseMasterSingle)
} | ||
|
||
async _connect () { | ||
this.knex = knex({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this is used for?
connection: this.writeConnection, | ||
}) | ||
|
||
this.knexMaster = knex({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming is a bit strange to me. It's usually master / replica or write / read, here is a combo :)
|
||
if (databaseUrl.startsWith('postgres')) { | ||
return keystone.adapter.listAdapters | ||
} else if (databaseUrl.startsWith('custom')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the custom db url?