Support integer shard keys

[HeyNonster]

Motivation / Background

Currently, .connects_to(shards: ...) coerces all of the shard keys into symbols. Because an integer cannot be coerced into a symbol, attempting to identify a shard with an integer will raise an exception:

1.to_sym
(irb):1:in '<main>': undefined method 'to_sym' for an instance of Integer (NoMethodError)

I think it would be useful to support integers as shard keys along with symbols. This would simplify shard switching when shards are identified by integer columns in a database.

As an example, if there is an accounts table with a shard integer column which identifies which shard that account's data lives on, this currently would not work:

account = Account.first

ActiveRecord::Base.connected_to(shard: account.shard) do
  # ...
end

A workaround would be to first coerce or serialize the integer into a string but that's unideal:

account = Account.first

ActiveRecord::Base.connected_to(shard: account.shard.to_s.to_sym) do
  # ...
end

Detail

This makes a .connects_to change, coercing the shard key into a symbol unless the key is an integer. From there, passing a symbol or integer to connected_to(shard: ...) should both work.

Additional information

connected_to will no longer raise NoMethodError when you pass it shard keys as integers.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Unrelated changes 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.

[byroot]

Agreed. Integer based shards are very common / useful. @eileencodes any objections?

[eileencodes]

I don't have a problem with this fundamentally however...this is going to sound picky and but seeing it written out it's kind of ugly, especially mixing symbols and integers like default: with 1 =>.

Here's my thought process:

So should 0 be default also so we're not mixing symbols and integers? Or should we not accept this apps generate a key name like shard_1 and then in the connects to it's connected_to(shard: "shard_#{account.shard}")? My issue is how with this change we're encouraging the mixing of symbols and integers and making the API potentially very ugly.

[HeyNonster]

I agree that it was pretty ugly. 🤮

I've updated the test to first call:

ActiveRecord::Base.default_shard = 0

It's normally :default, which necessitated mixing :default in with the integers. I imagine if folks are only using integers to identify shards then they would pick a one as a default shard and configure ActiveRecord::Base accordingly (or just use a separate abstract class).

I'd just like to avoid coercing the integer into a string and then into a symbol, but I also recognize I'm bumping into a Ruby constraint here.

[byroot]

So should 0 be default also so we're not mixing symbols and integers?

I don't think 0 should be considered a default. If anything, with numeric shards you want no default at all (enforce that one shard is always selected).

Or should we not accept this apps generate a key name like shard_1

This is what Shopify's monolith was doing, and I remember at one point the "shard_#{shard_id}" interpolation was the number one allocation source, so we had to add some cache and memoization etc.

Overall I like the purely numeric naming, it's practical.

[HeyNonster]

I don't think 0 should be considered a default. If anything, with numeric shards you want no default at all (enforce that one shard is always selected).

I agree. The other tests in the file were covering what happens when there is a default so I opted to cover that case. Happy to make changes though.

This is what Shopify's monolith was doing, and I remember at one point the "shard_#{shard_id}" interpolation was the number one allocation source, so we had to add some cache and memoization etc.

Yep, we're in a similar situation at Zendesk.