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

Adds master-slave capability to FinagleMysqlContext #1079

Merged
merged 4 commits into from May 1, 2018

Conversation

Projects
None yet
2 participants
@benpence
Contributor

benpence commented Apr 25, 2018

Problem

Deferring reads to a MySQL slave is not supported in finagle-mysql

Solution

Provide master & slave constructor arguments and perform all queries on slave.

Not sure whether we want to go the extra mile and add unit tests. I was thinking we could create 2 databases in the MySQL docker service and verify that writes were only reflected in the master and reads were only taking place from the slave. Would probably need to add a .sql file in the container's /docker-entrypoint-initdb.d (according to https://hub.docker.com/_/mysql/) to create a second database.

@getquill/maintainers

@benpence benpence changed the title from Finagle mysql master slave to Adds master-slave capability to FinagleMysqlContext Apr 25, 2018

@fwbrasil

This comment has been minimized.

Contributor

fwbrasil commented Apr 25, 2018

thank you @benpence! Yes, we need test coverage for this change. I think it'd be challenging to add a second database and it might make the CI even more unstable. Could you write unit tests using a stub for the finagle client?

@benpence

This comment has been minimized.

Contributor

benpence commented Apr 25, 2018

@fwbrasil OK added. Let me know if that's close to what you were looking for

@fwbrasil

This comment has been minimized.

Contributor

fwbrasil commented Apr 26, 2018

Thank for the tests! The compilation is failing, though. Basically it's not possible to reuse the qr4 since it is created by another context. You just need to replace it by query[TheEntityType].

�[0m[�[0m�[31merror�[0m] �[0m�[0m/app/quill-finagle-mysql/src/test/scala/io/getquill/context/finagle/mysql/FinagleMysqlContextSpec.scala:62:23: type mismatch;�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0m found   : io.getquill.context.finagle.mysql.testContext.EntityQuery[io.getquill.context.finagle.mysql.testContext.TestEntity4]�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0m required: context.EntityQuery[io.getquill.context.finagle.mysql.testContext.TestEntity4]�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0m    await(context.run(qr4.insert(TestEntity4(0))))�[0m
@benpence

This comment has been minimized.

Contributor

benpence commented Apr 26, 2018

Thanks @fwbrasil. It's weird because I successfully packaged it and published it locally to import into another project. Must have not committed all the changes. ¯\_(ツ)_/¯

@benpence

This comment has been minimized.

Contributor

benpence commented Apr 30, 2018

@fwbrasil Is the CI failure something I can fix? I'm not sure I understand the failure

@fwbrasil

This comment has been minimized.

Contributor

fwbrasil commented Apr 30, 2018

I'm trying to fix the CI

@fwbrasil

This comment has been minimized.

Contributor

fwbrasil commented Apr 30, 2018

@benpence coud you rebase your branch?

@benpence benpence force-pushed the benpence:finagle-mysql-master-slave branch from 1dcceb9 to 2e3dc64 Apr 30, 2018

@fwbrasil fwbrasil merged commit 1bf6027 into getquill:master May 1, 2018

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 100% of diff hit (target 95.44%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +4.55% compared to 271ebc3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fwbrasil fwbrasil referenced this pull request May 5, 2018

Merged

trigger 2.5.0 release #1087

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment