Subclass switching #136

jacobat · 2017-10-11T13:05:15Z

Another stab at making ARS a little more understandable. The gist of it is that we try to isolate the ActiveRecordShards logic in a class instead of monkey patching ActiveRecord::Base.

This PR changes a bunch of things around ActiveRecordShards. First up it drops support for Rails < 5. It also removes the migration parts of ARS as those can be better handled separately. It introduces a new ActiveRecordShards::ShardedModel class which sharded models must now inherit from instead of ActiveRecord::Base. On the flip side the not_sharded instruction is no longer necessary or supported. These changes should make ActiveRecord classes not inheriting from ActiveRecordShards::ShardedModel behave more like they would when not using ActiveRecordShards.

This also removes support for the default_shard idea as it doesn't make a lot of sense to talk about a default shard. If you want a random shard just use the first shard available.

Additionally this introduces the requirement that a shard must always be selected. It's no longer possible to say on_shard(nil). Only numeric shards are accepted.

Future work on this would be to separate sharding from master/slave logic and move the patches to ActiveRecord::Base down to the ActiveRecordShards::ShardedModel class making ActiveRecord::Base totally ignorant of ActiveRecordShards.

With these changes we'll need to release a new major version of ARS to comply with SemVer.

bquorning · 2017-10-11T20:03:19Z

lib/active_record_shards/default_slave_patches.rb

@@ -141,8 +141,6 @@ def model.on_slave_by_default?
          left_reflection.klass.on_slave_by_default?
        end

-        # also transfer the sharded-ness of the left table to the join model
-        model.not_sharded unless model.left_reflection.klass.is_sharded?


Will this be a problem?

Not sure :|

seems super sketchy that we call not_sharded ... maybe this is a 1-off join model ... which means that it should now be model.sharded if foo instead ?

Can we add a test for this case?

I've tried adding a test - it's not exactly obvious where to put it though

grosser · 2017-10-12T15:25:21Z

I'm a little scared that an inheritance change would break all existing usages and also introduce bugs where things like base_class no longer return the right classes ... prefer an include or a sharded method

grosser · 2017-10-12T15:28:04Z

lib/active_record_shards/connection_switcher.rb

+          end
+          unless config['shard_names'].include?(options[:shard])
+            raise "Did not find shard #{options[:shard]} in configurations"
+          end


these validations could go out as their own PR so we can test them quickly and make this PR smaller ?

Sure we can do that.

Except that options[:shard] can still be nil which cannot be found in config['shard_names']. So the validations need to be tweaked a bit.

Extracted to #143, backport to master in #144.

pschambacher · 2017-10-12T18:01:29Z

That's is one big and scary PR. Should we test it on one application before merging?

jacobat · 2017-10-12T18:13:57Z

I'm a little scared that an inheritance change would break all existing usages and also introduce bugs where things like base_class no longer return the right classes ... prefer an include or a sharded method

I considered making it a module one would extend as well. I can give it a shot. base_class should still return the right thing though as ShardedModel is an abstract class in AR terms.

This will be a new major version and will require changes to existing code so I'm not too worried about breaking existing usage.

jacobat · 2017-10-12T18:14:31Z

That's is one big and scary PR. Should we test it on one application before merging?

I imagine releasing it as ARS 4.0 beta and test it with the K8s HC branch.

pschambacher

👍 for beta tag

bquorning · 2017-10-23T13:02:39Z

lib/active_record_shards/sharded_model.rb

  class ShardedModel < ActiveRecord::Base
    self.abstract_class = true
+
+    extend ActiveRecordShards::Ext::ShardedModel


Why is this better than def self.is_sharded? ?

You mean why am I extending a module over just defining a method?

Yeah, that’s what I meant.

I need the module to extend into various ActiveRecord classes where I can't easily change the superclass (models from gems). As I need a module I might as well reuse the module here to keep it slightly more dry.

bquorning · 2017-10-25T10:20:47Z

lib/active_record_shards/connection_switcher.rb

          current_shard_selection.shard = options[:shard]
        end

+        if options.key?(:slave)
+          current_shard_selection.on_slave = options[:slave]


Do/should we support master/slave switching without switching shards?

I'm not sure what you're getting at?

Me neither. This is fine.

bquorning · 2017-10-25T10:24:30Z

lib/active_record_shards/model.rb

+      ActiveSupport::Deprecation.warn("Calling not_sharded is deprecated. "\
+                                      "Please ensure to still read from the "\
+                                      "account db slave after removing the "\
+                                      "call.")
    end

    def is_sharded? # rubocop:disable Naming/PredicateName


Should we seize the moment and rename .is_sharded? to .sharded??

bquorning

A few comments but 👍 and ❤️ to the overall plan.

grosser · 2017-10-25T10:32:14Z

prefer changes are nicer ... rename in old branch and bump in classic then fix everything up

…

On Wed, Oct 25, 2017 at 12:24 PM, Benjamin Quorning < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/active_record_shards/model.rb <#136 (comment)> : > @@ -3,24 +3,14 @@ module ActiveRecordShards module Model def not_sharded - if self != ActiveRecord::Base && self != base_class - raise "You should only call not_sharded on direct descendants of ActiveRecord::Base" - end - self.sharded = false + ActiveSupport::Deprecation.warn("Calling not_sharded is deprecated. "\ + "Please ensure to still read from the "\ + "account db slave after removing the "\ + "call.") end def is_sharded? # rubocop:disable Naming/PredicateName Should we seize the moment and rename .is_sharded? to .sharded?? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#136 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAsZ8X8yvcMmg_hJGKiYrolDc80oys4ks5svwxkgaJpZM4P1cOw> .

jacobat · 2017-10-25T10:38:53Z

I'm not sure what you're saying?

bquorning · 2017-10-25T10:41:58Z

I think it’s a threading fail. The … reveals my original question

Should we seize the moment and rename .is_sharded? to .sharded??

jacobat · 2017-10-25T10:44:14Z

Ah!

It does not make sense to have a default shard so we're removing this

We don't want to have a "selection" where nothing is selected as that may cause weird issues. For instance it may result in not being able to get the schema of sharded tables.

If we're using ARS we expect shards to be configured. We don't want to silently fail in case the configuration is missing.

If we're missing configuration we raise an exception at the point we discover the missing configuration. This is to avoid accidentally calling .shard= with nil.

We move the state manipulating on_slave= call to after we check that we're actually able to perform the shard switch. Otherwise we may accidentally update state and then see an exception leaving us in an inconsistent state.

When selecting a new shard the user should always select an actual shard. It does not make sense to select NO_SHARD.

When we ask for the shard we must ensure that we do have a shard selected.

It no longer makes sense to talk about being able to switch to shard and back as unsharded models will now always be unsharded and sharded models will now always be sharded.

I don't even know why we're testing behaviour of ARS in an unsharded environment.

From now on sharded models must inherit from ActiveRecordShards::ShardedModel - and no longer will users need to use the "not_sharded" instruction to mark models not sharded. By doing this we've simplified the logic around sharding as ActiveRecord::Base no longer needs to be part of the calculation around wether a class is sharded or not. It's now purely defined in terms of inheritance, inheriting from ShardedModel means the model is sharded.

This is no longer used

We are method chaining not_sharded in ZendeskDatabaseSupport and adding default reading from the slave for unsharded models. To help the transition we'll keep the not_sharded method around until we can remove it from the dependents.

Instead of checking class inheritance we can just call a class method. This way we can define it to be false for ActiveRecord::Base and override it in sharded subclasses. This also gives us the possibility of using a module (or just plain defining the method in any random class).

Global state in tests, yay

jacobat · 2017-10-26T10:56:44Z

I've merged #146 bumping to beta-4.0.0.beta1 on the v4 branch. Does anyone know how to release this onto rubygems.org?

bquorning · 2017-10-26T11:08:39Z

v4.0.0.beta1 has been released 🎉 ✨

jacobat force-pushed the jacobat/subclass-switching branch 2 times, most recently from 4ff1179 to f66c783 Compare October 11, 2017 14:34

bquorning reviewed Oct 11, 2017

View reviewed changes

bquorning force-pushed the jacobat/subclass-switching branch from f5f238f to c00a1b6 Compare October 11, 2017 20:04

jacobat force-pushed the jacobat/subclass-switching branch 4 times, most recently from d011f2a to a1a1f97 Compare October 12, 2017 13:55

jacobat requested review from grosser, bquorning and pschambacher October 12, 2017 14:11

jacobat mentioned this pull request Oct 12, 2017

HC k8s patches #130

Closed

grosser reviewed Oct 12, 2017

View reviewed changes

pschambacher approved these changes Oct 12, 2017

View reviewed changes

jacobat force-pushed the jacobat/subclass-switching branch from a1a1f97 to bfd516d Compare October 13, 2017 09:14

jacobat changed the base branch from master to v4 October 13, 2017 09:15

jacobat force-pushed the jacobat/subclass-switching branch 3 times, most recently from 587958a to 9d1182c Compare October 13, 2017 09:34

bquorning mentioned this pull request Oct 13, 2017

Add a few warnings to the connection switcher and conf parser #143

Merged

jacobat force-pushed the jacobat/subclass-switching branch 2 times, most recently from 9746156 to 9d1182c Compare October 23, 2017 09:42

bquorning reviewed Oct 23, 2017

View reviewed changes

jacobat force-pushed the jacobat/subclass-switching branch from edf44fd to 268e74d Compare October 24, 2017 10:43

bquorning reviewed Oct 25, 2017

View reviewed changes

bquorning approved these changes Oct 25, 2017

View reviewed changes

jacobat and others added 18 commits October 26, 2017 10:56

Deprecate default_shard=

00a9112

It does not make sense to have a default shard so we're removing this

Initialize ShardSelection with a shard

6e7af17

We don't want to have a "selection" where nothing is selected as that may cause weird issues. For instance it may result in not being able to get the schema of sharded tables.

Raise if configuration is missing

211cd5b

If we're using ARS we expect shards to be configured. We don't want to silently fail in case the configuration is missing.

Add more checking of available configuration

93e3e89

If we're missing configuration we raise an exception at the point we discover the missing configuration. This is to avoid accidentally calling .shard= with nil.

Fix state manipulation

7fffa92

We move the state manipulating on_slave= call to after we check that we're actually able to perform the shard switch. Otherwise we may accidentally update state and then see an exception leaving us in an inconsistent state.

Check shard selection

09695b0

When selecting a new shard the user should always select an actual shard. It does not make sense to select NO_SHARD.

Raise if no shard selected

0845ce4

When we ask for the shard we must ensure that we do have a shard selected.

Remove migrations from ARS

237ebd7

Make sql comments work

eab02f0

Remove meaningless test

8c83115

It no longer makes sense to talk about being able to switch to shard and back as unsharded models will now always be unsharded and sharded models will now always be sharded.

Remove meaningless test

61d88b6

I don't even know why we're testing behaviour of ARS in an unsharded environment.

Remove ActiveRecordShards::Model.not_sharded

93f67b8

Remove sharded accessor

c847cba

This is no longer used

Add not_sharded back in with a deprecation warning.

4cbafd5

We are method chaining not_sharded in ZendeskDatabaseSupport and adding default reading from the slave for unsharded models. To help the transition we'll keep the not_sharded method around until we can remove it from the dependents.

Handle HABTM for sharded models

54f6ae9

Fix randomly failing tests

db1760d

Global state in tests, yay

jacobat force-pushed the jacobat/subclass-switching branch from 4b0a309 to db1760d Compare October 26, 2017 09:02

bquorning approved these changes Oct 26, 2017

View reviewed changes

jacobat merged commit 2a27966 into v4 Oct 26, 2017

jacobat deleted the jacobat/subclass-switching branch October 26, 2017 09:18

bquorning added this to the v4 milestone Dec 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subclass switching #136

Subclass switching #136

jacobat commented Oct 11, 2017 •

edited

Loading

bquorning Oct 11, 2017

jacobat Oct 12, 2017

grosser Oct 12, 2017

bquorning Oct 24, 2017

jacobat Oct 24, 2017

grosser commented Oct 12, 2017

grosser Oct 12, 2017

jacobat Oct 12, 2017

bquorning Oct 13, 2017

bquorning Oct 25, 2017

pschambacher commented Oct 12, 2017

jacobat commented Oct 12, 2017

jacobat commented Oct 12, 2017

pschambacher left a comment

bquorning Oct 23, 2017

jacobat Oct 23, 2017

bquorning Oct 23, 2017 •

edited

Loading

jacobat Oct 23, 2017

bquorning Oct 25, 2017 •

edited

Loading

jacobat Oct 25, 2017

bquorning Oct 26, 2017

bquorning Oct 25, 2017

jacobat Oct 25, 2017

bquorning left a comment

grosser commented Oct 25, 2017 via email

jacobat commented Oct 25, 2017

bquorning commented Oct 25, 2017

jacobat commented Oct 25, 2017

jacobat commented Oct 26, 2017

bquorning commented Oct 26, 2017

Subclass switching #136

Subclass switching #136

Conversation

jacobat commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschambacher commented Oct 12, 2017

jacobat commented Oct 12, 2017

jacobat commented Oct 12, 2017

pschambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bquorning Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bquorning Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bquorning left a comment

Choose a reason for hiding this comment

grosser commented Oct 25, 2017 via email

jacobat commented Oct 25, 2017

bquorning commented Oct 25, 2017

jacobat commented Oct 25, 2017

jacobat commented Oct 26, 2017

bquorning commented Oct 26, 2017

jacobat commented Oct 11, 2017 •

edited

Loading

bquorning Oct 23, 2017 •

edited

Loading

bquorning Oct 25, 2017 •

edited

Loading