This repository has been archived by the owner on Nov 23, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 3
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Breaking change] This patch adds `auto_increment_increment` validation to the following scenarios: * Connection initalization * Generating one ID * Generating multiple IDs These changes should have no noticeable impact on performance and are thread safe. The intent is to prevent data corruption when a server is misconfigured and in the event that one is found, it's removed from the pool of available alloc servers allowing creation to continue with the valid alloc servers. Validation during initialization is done by comparing the client configuration and with what's set on each server. If the server is misconfigured, an error is reported via the `notifier` and it's left out of the pool. Validation during creation happens by comparing each pair of identifiers and ensuring they've been incremented correctly by the configured amount. If the `auto_increment_increment` changes while the clients are using the server, the alloc server is removed from the list of servers and an error is reported via the `notifier`. An InvalidIncrementException is raised if all servers are misconfigured. The changeset is considered a breaking change as any clients upgrading will see an increment exception if their `increment_by` was set incorrectly, this previously would have gone unnoticed. [squashing] Add more detail to what the spec is doing Addressing this comment > Could we please expand a bit on this comment? Maybe tell which IDs are > created now, and which IDs would be created after calling > updated_increment once or twice. > It feels like I have to read a lot of code to understand this test in > its current form. I could do that now, but I know that I may well be > to lazy to do so when looking at this test a year or two in the future Asserting the IDs could lead to spurious failures since it'll come from one of the two servers. [squashing] Rename max_size to max_window_size max_size is misleading, one may presume that it's the maximum amount of IDs allowed to be allocated for the server. If they passed 100000 in, it would have a negative performance hit [squashing] Rename allocation methods The method names were inconsistent, the method checks the allocation, comparing it with the previous. It doesn't check the increment, except in `validate_connection_increment` which remains unchanged [squashing] Cleanup allocator validation The previous implementation was a mess, it was cleaned up in the follow- up PR, bringing it strait into this PR. The InvalidIncrementException message has also been improved, when the ID returned is an unexpected value, we'll query the database before closing the connection to get the set `auto_increment_increment` value. E.g GlobalUid::InvalidIncrementException Configured: '5', Found: '42' on 'global_uid_test_id_server_1'. Recently allocated IDs: [43, 85] Also, while re-reading the code I realised that the previous `alert` implementation would log the error twice when exceptions aren't suppressed
- Loading branch information
Showing
7 changed files
with
377 additions
and
35 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| module GlobalUid | ||
| class Allocator | ||
| attr_reader :recent_allocations, :max_window_size, :incrementing_by, :connection | ||
|
|
||
| def initialize(incrementing_by:, connection:) | ||
| @recent_allocations = [] | ||
| @max_window_size = 5 | ||
| @incrementing_by = incrementing_by | ||
| @connection = connection | ||
| validate_connection_increment | ||
| end | ||
|
|
||
| def allocate_one(table) | ||
| identifier = connection.insert("REPLACE INTO #{table} (stub) VALUES ('a')") | ||
| allocate(identifier) | ||
| end | ||
|
|
||
| def allocate_many(table, count:) | ||
| increment_by = validate_connection_increment | ||
|
|
||
| start_id = connection.insert("REPLACE INTO #{table} (stub) VALUES " + (["('a')"] * count).join(',')) | ||
| identifiers = start_id.step(start_id + (count - 1) * increment_by, increment_by).to_a | ||
| identifiers.each { |identifier| allocate(identifier) } | ||
| identifiers | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def allocate(identifier) | ||
| recent_allocations.shift if recent_allocations.size >= max_window_size | ||
| recent_allocations << identifier | ||
|
|
||
| if !valid_allocation? | ||
| db_increment = connection.select_value("SELECT @@auto_increment_increment") | ||
| message = "Configured: '#{incrementing_by}', Found: '#{db_increment}' on '#{connection.current_database}'. Recently allocated IDs: #{recent_allocations}" | ||
| alert(InvalidIncrementException.new(message)) | ||
| end | ||
|
|
||
| identifier | ||
| end | ||
|
|
||
| def valid_allocation? | ||
| recent_allocations[1..-1].all? do |identifier| | ||
| (identifier > recent_allocations[0]) && | ||
| (identifier - recent_allocations[0]) % incrementing_by == 0 | ||
| end | ||
| end | ||
|
|
||
| def validate_connection_increment | ||
| db_increment = connection.select_value("SELECT @@auto_increment_increment") | ||
|
|
||
| if db_increment != incrementing_by | ||
| alert(InvalidIncrementException.new("Configured: '#{incrementing_by}', Found: '#{db_increment}' on '#{connection.current_database}'")) | ||
| end | ||
|
|
||
| db_increment | ||
| end | ||
|
|
||
| def alert(exception) | ||
| if GlobalUid::Base.global_uid_options[:suppress_increment_exceptions] | ||
| GlobalUid::Base.notify(exception, exception.message) | ||
| else | ||
| raise exception | ||
| end | ||
| end | ||
| end | ||
| end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| # frozen_string_literal: true | ||
| require_relative '../test_helper' | ||
|
|
||
| describe GlobalUid::Allocator do | ||
| let(:connection) { mock('connection') } | ||
| let(:increment_by) { 10 } | ||
| let(:allocator) { GlobalUid::Allocator.new(incrementing_by: increment_by, connection: connection) } | ||
| let(:table) { mock('table_class').class } | ||
|
|
||
| before do | ||
| restore_defaults! | ||
| connection.stubs(:current_database).returns('database_name') | ||
| end | ||
|
|
||
| describe 'with a configured increment_by that differs from the connections auto_increment_increment' do | ||
| it 'raises an exception to prevent erroneous ID allocation' do | ||
| connection.stubs(:select_value).with('SELECT @@auto_increment_increment').returns(50) | ||
| exception = assert_raises(GlobalUid::InvalidIncrementException) do | ||
| GlobalUid::Allocator.new(incrementing_by: 10, connection: connection) | ||
| end | ||
|
|
||
| assert_equal("Configured: '10', Found: '50' on 'database_name'", exception.message) | ||
| end | ||
| end | ||
|
|
||
| describe 'with a configured increment_by that matches the connections auto_increment_increment' do | ||
| before do | ||
| connection.stubs(:select_value).with('SELECT @@auto_increment_increment').returns(increment_by) | ||
| end | ||
|
|
||
| describe '#allocate_one' do | ||
| it 'allocates IDs, maintaining a small rolling selection of IDs for comparison' do | ||
| [10, 20, 30, 40, 50, 60, 70, 80].each do |id| | ||
| connection.expects(:insert).returns(id) | ||
| allocator.allocate_one(table) | ||
| end | ||
|
|
||
| assert_equal(5, allocator.max_window_size) | ||
| assert_equal([40, 50, 60, 70, 80], allocator.recent_allocations) | ||
| end | ||
|
|
||
| describe 'gap between ID not divisible by increment_by' do | ||
| it 'raises an error' do | ||
| connection.expects(:insert).returns(20) | ||
| allocator.allocate_one(table) | ||
|
|
||
| connection.stubs(:select_value).with('SELECT @@auto_increment_increment').returns(5) | ||
| connection.expects(:insert).returns(25) | ||
| exception = assert_raises(GlobalUid::InvalidIncrementException) do | ||
| allocator.allocate_one(table) | ||
| end | ||
|
|
||
| assert_equal("Configured: '10', Found: '5' on 'database_name'. Recently allocated IDs: [20, 25]", exception.message) | ||
| end | ||
| end | ||
|
|
||
| describe 'ID value does not increment upwards' do | ||
| it 'raises an error' do | ||
| connection.expects(:insert).returns(20) | ||
| allocator.allocate_one(table) | ||
|
|
||
| connection.expects(:insert).returns(20 - increment_by) | ||
| exception = assert_raises(GlobalUid::InvalidIncrementException) do | ||
| allocator.allocate_one(table) | ||
| end | ||
|
|
||
| assert_equal("Configured: '10', Found: '10' on 'database_name'. Recently allocated IDs: [20, 10]", exception.message) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe '#allocate_many' do | ||
| it 'allocates IDs, maintaining a small rolling selection of IDs for comparison' do | ||
| connection.expects(:insert) | ||
| .with("REPLACE INTO Mocha::Mock (stub) VALUES ('a'),('a'),('a'),('a'),('a'),('a'),('a'),('a')") | ||
| .returns(10) | ||
| allocator.allocate_many(table, count: 8) | ||
|
|
||
| assert_equal(5, allocator.max_window_size) | ||
| assert_equal([40, 50, 60, 70, 80], allocator.recent_allocations) | ||
| end | ||
|
|
||
| describe 'with a configured increment_by that differs from the active connection auto_increment_increment' do | ||
| it 'raises an error' do | ||
| connection.stubs(:select_value).with('SELECT @@auto_increment_increment').returns(5) | ||
| connection.expects(:insert).never | ||
| exception = assert_raises(GlobalUid::InvalidIncrementException) do | ||
| allocator.allocate_many(table, count: 8) | ||
| end | ||
|
|
||
| assert_equal("Configured: '10', Found: '5' on 'database_name'", exception.message) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
Oops, something went wrong.