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.

Sign up for GitHub

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

Jump to bottom

Introduce adapter for Trilogy, a MySQL-compatible DB client #47880

Merged
merged 1 commit into from
Apr 17, 2023
Merged

Introduce adapter for Trilogy, a MySQL-compatible DB client #47880

merged 1 commit into from
Apr 17, 2023

Conversation

adrianna-chang-shopify
Copy link
Contributor

Motivation / Background

The Trilogy database client and corresponding Active Record adapter were both open sourced by GitHub last year. Shopify adopted Trilogy successfully in our Rails monolith several weeks ago.

With two major Rails applications running Trilogy successfully, we'd like to propose upstreaming the adapter to Rails as a MySQL-compatible alternative to Mysql2Adapter.

This effort is a collaboration between @adrianna-chang-shopify and @eileencodes (Shopify) and @matthewd (GitHub). Original authors of the adapter library are credited.

Detail

This PR ports over the adapter code and all of the test cases from https://github.com/github/activerecord-trilogy-adapter. We can DRY some of the code up between Trilogy and the existing Mysql2 adapter, but I'm proposing we copy everything verbatim for now to ensure full test coverage, and clean things up in a subsequent PR.

Additional changes made:

  • Added options support to #explain on the TrilogyAdapter for parity with Mysql2
  • Database generator supports trilogy option for applications
  • Migration compatibility added for Trilogy
  • TrilogyAdapter#select_all updated to ensure connection returned to a good state after MULTI_RESULT query
  • Active Record Rakefile updated to run abstract MySQL test cases against Trilogy

Additional information

This is blocked by trilogy-libraries/trilogy#64 on the client side. Once that ships, we should release a new version of the client and update the Gemfile to match.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@eileencodes eileencodes self-assigned this Apr 6, 2023
@eileencodes eileencodes added this to the 7.1.0 milestone Apr 6, 2023
eileencodes added a commit to rails/buildkite-config that referenced this pull request Apr 7, 2023
@eileencodes
Add build for trilogy client
0aec026 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.
eileencodes added a commit to rails/buildkite-config that referenced this pull request Apr 7, 2023
@eileencodes
Add build for trilogy client
6964ac1 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.
eileencodes added a commit to rails/buildkite-config that referenced this pull request Apr 7, 2023
@eileencodes
Add build for trilogy client
720b136 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.
@zzak
Copy link
Member

zzak commented Apr 8, 2023

tumblr_m0i88xhqiM1qjgw3wo1_500

Thanks everyone who contributed! This is definitely better than the prequel.

eileencodes added a commit to rails/buildkite-config that referenced this pull request Apr 10, 2023
@eileencodes
Add build for trilogy client
d9b36fe 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.
@eileencodes
Copy link
Member

Ok the trilogy build is working now, and I fixed some Railties tests. We're green here but locally I noticed a couple flakey tests in mysql2 and trilogy (same ones) regarding closed connections. If I get a chance I'll take a deeper look but I'm not sure they are flaky on CI - might just be locally.

@eileencodes
Copy link
Member

We're green here but locally I noticed a couple flakey tests in mysql2 and trilogy (same ones) regarding closed connections

I can't reproduce this now so it must have been something else wrong. So I think other then the encoding work this is good to go. I asked the rest of core to take a look to make sure I'm not missing anything. Great work @adrianna-chang-shopify!

@adrianna-chang-shopify
Copy link
Contributor Author

Encoding work is shipped! We should also ship the changes to the SyscallErrors in the client to make sure all of the client errors can be rescued as Trilogy::Error.

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-trilogy-adapter branch 3 times, most recently from 13604c6 to 224d03e Compare April 17, 2023 15:32
@adrianna-chang-shopify @tenderlove
@aroben @alimi @tmm1 @arthurschreiber @bkeepers @brianmario @bryanaknight @chrisbloom7 @cpruitt @composerinteralia @calavera @mockra @dbussink @eileencodes @enriikke @HParker @haileys @jdennes @janester @TooManyBees @joelhawksley @jbarnette @seejohnrun @jhawthorn @jnunemaker @jonmagic @keeran @ksolo @Tonkpils @LizzHale @lorint @al2o3cr @matthewd @zerowidth @nholden @paarthmadan @rsanheim @shayfrendt @tma @TwP @ymendel @zkoppert @cryptoque
Introduce adapter for Trilogy, a MySQL-compatible DB client
5ed3f60 
The [Trilogy database client][trilogy-client] and corresponding
[Active Record adapter][ar-adapter] were both open sourced by GitHub last year.

Shopify has recently taken the plunge and successfully adopted Trilogy in their Rails monolith.
With two major Rails applications running Trilogy successfully, we'd like to propose upstreaming the adapter
to Rails as a MySQL-compatible alternative to Mysql2Adapter.

[trilogy-client]: https://github.com/github/trilogy
[ar-adapter]: https://github.com/github/activerecord-trilogy-adapter

Co-authored-by: Aaron Patterson <tenderlove@github.com>
Co-authored-by: Adam Roben <adam@roben.org>
Co-authored-by: Ali Ibrahim <aibrahim2k2@gmail.com>
Co-authored-by: Aman Gupta <aman@tmm1.net>
Co-authored-by: Arthur Nogueira Neves <github@arthurnn.com>
Co-authored-by: Arthur Schreiber <arthurschreiber@github.com>
Co-authored-by: Ashe Connor <kivikakk@github.com>
Co-authored-by: Brandon Keepers <brandon@opensoul.org>
Co-authored-by: Brian Lopez <seniorlopez@gmail.com>
Co-authored-by: Brooke Kuhlmann <brooke@testdouble.com>
Co-authored-by: Bryana Knight <bryanaknight@github.com>
Co-authored-by: Carl Brasic <brasic@github.com>
Co-authored-by: Chris Bloom <chrisbloom7@github.com>
Co-authored-by: Cliff Pruitt <cliff.pruitt@cliffpruitt.com>
Co-authored-by: Daniel Colson <composerinteralia@github.com>
Co-authored-by: David Calavera <david.calavera@gmail.com>
Co-authored-by: David Celis <davidcelis@github.com>
Co-authored-by: David Ratajczak <david@mockra.com>
Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
Co-authored-by: Eileen Uchitelle <eileencodes@gmail.com>
Co-authored-by: Enrique Gonzalez <enriikke@gmail.com>
Co-authored-by: Garrett Bjerkhoel <garrett@github.com>
Co-authored-by: Georgi Knox <georgicodes@github.com>
Co-authored-by: HParker <HParker@github.com>
Co-authored-by: Hailey Somerville <hailey@hailey.lol>
Co-authored-by: James Dennes <jdennes@gmail.com>
Co-authored-by: Jane Sternbach <janester@github.com>
Co-authored-by: Jess Bees <toomanybees@github.com>
Co-authored-by: Jesse Toth <jesse.toth@github.com>
Co-authored-by: Joel Hawksley <joelhawksley@github.com>
Co-authored-by: John Barnette <jbarnette@github.com>
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
Co-authored-by: John Hawthorn <john@hawthorn.email>
Co-authored-by: John Nunemaker <nunemaker@gmail.com>
Co-authored-by: Jonathan Hoyt <hoyt@github.com>
Co-authored-by: Katrina Owen <kytrinyx@github.com>
Co-authored-by: Keeran Raj Hawoldar <keeran@gmail.com>
Co-authored-by: Kevin Solorio <soloriok@gmail.com>
Co-authored-by: Leo Correa <lcorr005@gmail.com>
Co-authored-by: Lizz Hale <lizzhale@github.com>
Co-authored-by: Lorin Thwaits <lorint@gmail.com>
Co-authored-by: Matt Jones <al2o3cr@gmail.com>
Co-authored-by: Matthew Draper <matthewd@github.com>
Co-authored-by: Max Veytsman <mveytsman@github.com>
Co-authored-by: Nathan Witmer <nathan@zerowidth.com>
Co-authored-by: Nick Holden <nick.r.holden@gmail.com>
Co-authored-by: Paarth Madan <paarth.madan@shopify.com>
Co-authored-by: Patrick Reynolds <patrick.reynolds@github.com>
Co-authored-by: Rob Sanheim <rsanheim@gmail.com>
Co-authored-by: Rocio Delgado <rocio@github.com>
Co-authored-by: Sam Lambert <sam.lambert@github.com>
Co-authored-by: Shay Frendt <shay@github.com>
Co-authored-by: Shlomi Noach <shlomi-noach@github.com>
Co-authored-by: Sophie Haskins <sophaskins@github.com>
Co-authored-by: Thomas Maurer <tma@github.com>
Co-authored-by: Tim Pease <tim.pease@gmail.com>
Co-authored-by: Yossef Mendelssohn <ymendel@pobox.com>
Co-authored-by: Zack Koppert <zkoppert@github.com>
Co-authored-by: Zhongying Qiao <cryptoque@users.noreply.github.com>
matthewd
matthewd approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@matthewd matthewd left a comment

Choose a reason for hiding this comment

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

🚀 🙌🏻

We can DRY some of the code up between Trilogy and the existing Mysql2 adapter, but I'm proposing we copy everything verbatim for now to ensure full test coverage, and clean things up in a subsequent PR.

👍🏻

@eileencodes eileencodes merged commit f7a4022 into rails:main Apr 17, 2023
@eileencodes eileencodes deleted the ac-trilogy-adapter branch April 17, 2023 18:57
eileencodes added a commit to rails/buildkite-config that referenced this pull request Apr 18, 2023
@eileencodes
Add build for trilogy client
edbc3f0 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.

The build is scoped to Rails 7.1 and above since we will not support
Trilogy for older versions of Rails.
eileencodes added a commit to rails/buildkite-config that referenced this pull request Apr 18, 2023
@eileencodes
Add build for trilogy client
f68065d 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.

The build is scoped to Rails 7.1 and above since we will not support
Trilogy for older versions of Rails.
rmehner added a commit to rmehner/dockerfile-rails that referenced this pull request Jun 22, 2023
@rmehner
Detect usage of trilogy and adapt Dockerfile & docker-compose accordi…
1986e30 
…ngly

Trilogy has been in production use for quite some time and will see official Rails
support soon: rails/rails#47880

This makes sure we detect the usage of trilogy and adapt the output accordingly. We also
skip installing the libmysql-dev headers, since they're not needed with trilogy.
@rmehner rmehner mentioned this pull request Jun 22, 2023
Merged
@tmimura39 tmimura39 mentioned this pull request Sep 4, 2023
Closed
@ardecvz ardecvz mentioned this pull request Sep 27, 2023
Open
3 tasks
@jonathanhefner jonathanhefner mentioned this pull request Nov 24, 2023
Merged
@maxKimoby maxKimoby mentioned this pull request Jan 31, 2024
Closed
@mrudult mrudult mentioned this pull request Feb 8, 2024
Merged
atosbucket added a commit to atosbucket/rails-buildkit that referenced this pull request Mar 22, 2024
@atosbucket
Add build for trilogy client
74462bf 
In rails/rails#47880 we're adding Trilogy as an
additional mysql client for Rails. This change adds a new build for it.

The build is scoped to Rails 7.1 and above since we will not support
Trilogy for older versions of Rails.
@ravangen ravangen mentioned this pull request Apr 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewd matthewd matthewd approved these changes

Assignees

@eileencodes eileencodes

Labels
activerecord
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@adrianna-chang-shopify @zzak @eileencodes @matthewd