auto_increment_increment validation

[mriddle]

This patch adds auto_increment_increment validation to the following scenarios:

• Connection initialization
• Allocating one ID
• Allocating multiple IDs

A lot of refactoring was done as part of this PR and has either been merged separately or moved to #70

These changes have been benchmarked, should have no noticeable impact to performance and continue to be thread safe.

Validation during initialization is done by comparing the client configuration (increment_by) with each alloc server (auto_increment_increment). If the server is misconfigured, an error is reported via the notifier (defaults to error logs) and it's removed from the pool. The connection will be retried every 10 minutes (configurable via the connection_retry) indefinitely.

Validation during allocation happens by storing the ID returned and comparing it with the last 4 identifiers, ensuring they've been incremented correctly by the configured amount. The alloc servers are shared and there's no guarantee that the process will get the next increment. As an example, one process (think unicorn worker or puma thread) may receive 5, 15, 25 while the other received 10 & 20. This is why we validate by checking the remained. This will not raise an error if the auto_increment_increment increases by a factor of 2.

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. The server is retried after a minute, indefinitely.

An exception is raised and ID allocation ceases if all servers are misconfigured. This will result in the process raising a NoServersAvailableException exception and halting. New unicorns or puma threads will attempt to connect each time they are spawned.

To support valid updates to the auto_increment_increment on active alloc servers, the suppress_increment_exceptions option can be used to swallow the exceptions and call the notifier. This will allow those who have monitors configured to be notified of the change and also allow the servers to remain in use while the update is being performed.

Related:

• https://github.com/zendesk/zendesk_database_scala/pull/72

[ggrossman]

Separate issue that we should come back for later: Timeout.timeout is bad mojo and we should do the query timeout some other way

[mriddle]

@ggrossman perhaps we should have the alloc server connections set the wait_timeout MySQL session variable to the configured self.global_uid_options[:query_timeout]

That will remove the need for us to manage a timeout here, and the bad mojo that comes with it.

E.g. in the database.yml

test_id_server_1:
  <<: *MYSQL
  database: global_uid_test_id_server_1
  variables:
    auto_increment_increment: 5
    auto_increment_offset: 1
    max_execution_time: 10

test_id_server_2:
  <<: *MYSQL
  database: global_uid_test_id_server_2
  variables:
    auto_increment_increment: 5
    auto_increment_offset: 2
    max_execution_time: 10

That avoids the need for this gem to mess around with connection variables too, giving the client complete control.

EDIT: Ah, nvm. max_execution_time is only supported in MySQL 5.7

[ggrossman]

That's a good idea. I'm in favor of that. We should document that the alloc server connections should definitely declare an appropriate wait_timeout.

[ggrossman]

I think it could wait for another PR but it's a good idea to get rid of Timeout.timeout. It can cause entirely unpredictable badness if it actually triggers.

[mriddle]

Thanks for the feedback @ggrossman! I pushed the draft PR up early for visibility, sorry, I should have been more clear that it was a still a work in progress and I intended to make more changes :)

[bquorning]

I wonder if we should make separate PRs of each of the three “remove” commits:

• Remove the concept of dry_run
• Remove ServerVariables module
• Remove unused and often misleading options

[mriddle]

I forgot to mention that this work along with the work below is targeted for a 4.0.0 gem release (milestone) since the gem is undergoing significant change.

• Remove the concept of dry_run #64 Remove the concept of dry_run
• Add changelog #65 Remove GlobalUid::ServerVariables module
• Remove unused and often misleading options #68 Remove unused and misleading options
• Refactor: Promote configuration, notifier and server_info to classes. #70 Overhaul, updates to configuration and connection handling

[bquorning]

So many comments. Sorry.

[mriddle]

Addressed feedback, squashed and force pushed, thanks for the solid review 🙏

[mriddle]

I'll leave it open for another 48 hours in case others want to provide feedback before merging.

[mriddle]

Addressed feedback, squashed and force pushed. Thanks

[mriddle]

Updated the commit message to reflect the final state