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

feat(core): allow forcing write connection via forceWriteConnection #2838

Merged
merged 2 commits into from
Mar 1, 2022
Merged

feat(core): allow forcing write connection via forceWriteConnection #2838

merged 2 commits into from
Mar 1, 2022

Conversation

oliversalzburg
Copy link
Contributor

@oliversalzburg oliversalzburg commented Mar 1, 2022

This allows to bypass replica read attempts through the FindOptions, by setting the forceWriteConnection flag.

The intent is to allow users to force reads onto a master, if the user assumes that an entity has not been replicated yet.

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2022

Codecov Report

Merging #2838 (bcc35b7) into master (1584850) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2838   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          194       194           
  Lines        11734     11734           
  Branches      2700      2701    +1     
=========================================
  Hits         11734     11734           
Impacted Files Coverage Δ
packages/core/src/drivers/IDatabaseDriver.ts 100.00% <ø> (ø)
packages/knex/src/AbstractSqlDriver.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1584850...bcc35b7. Read the comment docs.

tests/features/replica-selection.test.ts Outdated Show resolved Hide resolved
docs/docs/read-connections.md Outdated Show resolved Hide resolved
tests/features/replica-selection.test.ts Outdated Show resolved Hide resolved
tests/features/replica-selection.test.ts Outdated Show resolved Hide resolved
docs/docs/read-connections.md Outdated Show resolved Hide resolved
@B4nan
Copy link
Member

B4nan commented Mar 1, 2022

please dont rebase after you get a review, so we maintain the history

@oliversalzburg
Copy link
Contributor Author

Are the commits ultimately squashed before merging or does every single commit actually have to make sense?

@B4nan
Copy link
Member

B4nan commented Mar 1, 2022

Yes, only one commit will go to master and the commit message is based on the PR title (and I can override it when merging).

@oliversalzburg
Copy link
Contributor Author

Understood. I'll try keep better care in the future. I just had a little fight with commitlint and figured I'd quickly amend the commit.

@B4nan
Copy link
Member

B4nan commented Mar 1, 2022

Yeah not a big deal for such a small PR, but I started to say this out load when I see it, otherwise things never change :]

@B4nan B4nan changed the title feat(core): explicit EM write connection selection feat(core): allow forcing write connection via forceWriteConnection Mar 1, 2022
@B4nan B4nan merged commit 36d1969 into mikro-orm:master Mar 1, 2022
@oliversalzburg oliversalzburg deleted the feat/force-master-read branch March 1, 2022 16:04
@asiFarran
Copy link
Contributor

@B4nan @oliversalzburg, in our application the vast majority of queries should run against the master in order to support immediate-read-after-write scenarios, but we do want to use read-replicas for a few specific use-cases (e.g. customer search or analytics queries).

Using the pattern above we'd have to add forceWriteConnection : true to almost all queries which is .... you get it.

I would like to propose the following (more generic) pattern:

  1. Add a useReadReplicasByDefault?: boolean to Configuration. This defaults to true so no breaking change and existing behavior is maintained.
  2. Replace forceWriteConnection on FindOptions with a useConnection?: 'read' | 'write' property that lets the developer be specific in both directions
  3. Use both the configurable default and the optional explicit setting to choose the connection.

This way the connection resolution strategy is completely configurable and can support all scenarios.

Does this sound reasonable to you ? I'm happy to open a PR for it

@mikro-orm mikro-orm deleted a comment from asiFarran Mar 10, 2022
@B4nan
Copy link
Member

B4nan commented Mar 10, 2022

Sounds good to me, PR welcome. Given this PR is not yet shipped in stable, I don't mind replacing the newly added forceWriteConnection with useConnection.

@oliversalzburg
Copy link
Contributor Author

Awesome change btw :)

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.

None yet

4 participants