trilogy-libraries / activerecord-trilogy-adapter

Active Record adapter for the Trilogy database client for Rails v6.0 - v7.0.
https://github.com/trilogy-libraries/trilogy
MIT License
172 stars 17 forks source link

Support Rails 7.0 #26

Closed lorint closed 1 year ago

lorint commented 1 year ago

Based on feedback from @bensheldon and @matthewd in #20, here is a new PR to bring support for ActiveRecord 7.0 to ARTA. Key goals:

lorint commented 1 year ago

... comments that could help speed up the process a bit 😁 👍

Thank you, Dhruv! I'll take time later this evening to implement your suggestions. I'm just one time zone away -- Nottingham, UK -- so should be updated by early tomorrow morning your time.

dhruvCW commented 1 year ago

@lorint that looks awesome 🍻 , I think following the same thought process of splitting controversial changes into their own commits could you split code related to @verified flag out into it's own commit ?

lorint commented 1 year ago

could you split code related to @verified flag out into it's own commit ?

Some of the key reasons that Trilogy is interesting boils down to its reliable nature under load -- specifically how it does auto-reconnects. Much of this behaviour is possible due to AR 7.1's new @verified flag, along with other related enhancements in ConnectionAdapters. This is one reason that both Github and Shopify run their environments on Edge Rails, and have worked together to make Trilogy great. If we remove @verified then it would change quite a bit of the fundamental nature of this driver, and require that a lot of test cases be skipped -- essentially everything that has to do with auto-reconnect.

To create a more "vanilla" AR 7.0 option, I would recommend a fork of this driver where all of those enhancements are removed, paring back the majority of the auto-reconnect tests. Would end up being quite a bit more trim, and likely run just fine in smaller shops -- places that don't use Percona / Vitess / etc. I could create this kind of "ARTA Lite for AR 7.0" version if that sounds interesting.

dhruvCW commented 1 year ago

Some of the key reasons that Trilogy is interesting boils down to its reliable nature under load -- specifically how it does auto-reconnects. Much of this behaviour is possible due to AR 7.1's new @verified flag, along with other related enhancements in ConnectionAdapters. This is one reason that both Github and Shopify run their environments on Edge Rails, and have worked together to make Trilogy great. If we remove @verified then it would change quite a bit of the fundamental nature of this driver, and require that a lot of test cases be skipped -- essentially everything that has to do with auto-reconnect.

Don't get me wrong I would love to keep the PR as is, I just don't want this delayed 😅 , personally the value for me also comes from the lack of dependencies and async API but the reconnection logic is indeed a massive plus.

lorint commented 1 year ago

I just don't want this delayed

Love your zeal! Interested in having feedback from anyone willing to try this out in a QA or production environment. Anyone out there ready to put this in their Gemfile to give it a real-world spin? :)

gem 'activerecord-trilogy-adapter', git: 'https://github.com/lorint/activerecord-trilogy-adapter.git',
                                    ref: '17fba11ede4d5efdda3609af00fed85fdf5580dc'
dhruvCW commented 1 year ago

Love your zeal! Interested in having feedback from anyone willing to try this out in a QA or production environment. Anyone out there ready to put this in their Gemfile to give it a real-world spin? :)


gem 'activerecord-trilogy-adapter', git: 'https://github.com/lorint/activerecord-trilogy-adapter.git',
branch: 'support_rails_7',
ref: 'd132990800ca9196b758e54f37c522a888b396fc'


so funny thing, I tried `bundle exec rails db:create` on a mysql 5.7 server locally with root username and an empty password and it just failed with unknown db auth error 😅 

I think I am missing some steps here 
lorint commented 1 year ago

unknown db auth error

From terminal run mysql -uroot and then this, changing out cool_username and my_database:

CREATE USER cool_username@localhost IDENTIFIED BY '';
CREATE DATABASE my_database;
GRANT ALL PRIVILEGES ON my_database.* TO cool_username@localhost;
QUIT

Change the username and database name to be what you have in your database.yml, and no need to do a db:create, only db:migrate / etc. From there it's likely that all will be well 😃

dhruvCW commented 1 year ago

From terminal run mysql -uroot and then this, changing out cool_username and my_database:

thanks but does this mean trilogy cannot be used to create a database and load a schema ?

PS: seems to only fail in db:create otherwise everything else seems to work just fine, would be interesting to know what database creation is not supported though not really a pressing issue 😁

lorint commented 1 year ago

trilogy cannot be used to create a database

Trilogy can be used, sure -- this is more of a MySQL configuration thing we're talking about, so would recommend moving this convo to Rails discussion -- create a topic out here and we can find a solution. I'll make a quick screencast about how to configure your root user so that you can do a db:create: https://discuss.rubyonrails.org/

lorint commented 1 year ago

@matthewd and @bensheldon -- what do you think about this commit which now converts this gem into being solely a backwards-compaible offering for Rails 7.0?

lorint commented 1 year ago

let's do it!

Yay!

Big thanks out to you and the team for all the support along the way. Thrilled that backwards-compatibility for this adapter will be available at the same time that it is formally introduced in 7.1!

bensheldon commented 1 year ago

This PR has been released: https://github.com/trilogy-libraries/activerecord-trilogy-adapter/releases/tag/v3.0.0

@lorint thank you 🎉

lorint commented 1 year ago

This is fantastic!

If there is interest, I've put together a PR with the minor tidbits necessary to offer AR 6.1 and 6.0 support.

Looking forward to seeing many folks use Trilogy in their projects.