From 9f51314bb5f6a694000b75c869ef44ce6b10c331 Mon Sep 17 00:00:00 2001 From: Matthew Riddle Date: Thu, 2 Apr 2020 13:27:44 +0200 Subject: [PATCH 1/2] Remove unnecessary references to `options` The options passed in are just the `global_uid_options`, refer to them directly to make the code easier to follow. --- lib/global_uid/base.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/global_uid/base.rb b/lib/global_uid/base.rb index 64177cc..5680d3b 100644 --- a/lib/global_uid/base.rb +++ b/lib/global_uid/base.rb @@ -72,9 +72,10 @@ def self.new_connection(name, connection_timeout) def self.init_server_info(options) id_servers = self.global_uid_servers + increment_by = self.global_uid_options[:increment_by] raise "You haven't configured any id servers" if id_servers.nil? or id_servers.empty? - raise "More servers configured than increment_by: #{id_servers.size} > #{options[:increment_by]} -- this will create duplicate IDs." if id_servers.size > options[:increment_by] + raise "More servers configured than increment_by: #{id_servers.size} > #{increment_by} -- this will create duplicate IDs." if id_servers.size > increment_by id_servers.map do |name, i| info = {} @@ -92,7 +93,7 @@ def self.disconnect! end def self.setup_connections!(options) - connection_timeout = options[:connection_timeout] + connection_timeout = self.global_uid_options[:connection_timeout] increment_by = options[:increment_by] if self.servers.nil? @@ -109,7 +110,7 @@ def self.setup_connections!(options) connection = new_connection(info[:name], connection_timeout) info[:cx] = connection - info[:retry_at] = Time.now + options[:connection_retry] if connection.nil? + info[:retry_at] = Time.now + self.global_uid_options[:connection_retry] if connection.nil? end end @@ -120,7 +121,7 @@ def self.with_connections(options = {}) options = self.global_uid_options.merge(options) servers = setup_connections!(options) - if !options[:per_process_affinity] + if !self.global_uid_options[:per_process_affinity] servers = servers.sort_by { rand } #yes, I know it's not true random. end From a56b31226981bb87d2ca7654a6e2685b3671817b Mon Sep 17 00:00:00 2001 From: Matthew Riddle Date: Thu, 2 Apr 2020 13:30:52 +0200 Subject: [PATCH 2/2] Removed unused `options` parameter In the following cases we were passing options around but never using them, introduced in the original implementation but never used. I found them misleading as whatever you passed in would ultimately be ignored. I've removed them so the code is more explicit and hopefully a little less confusing. --- Changelog.md | 1 + lib/global_uid/active_record_extension.rb | 8 ++++---- lib/global_uid/base.rb | 20 +++++++++----------- lib/global_uid/migration_extension.rb | 2 +- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/Changelog.md b/Changelog.md index e897993..81c072f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Removed - Removed the `dry_run` option (https://github.com/zendesk/global_uid/pull/64) - Removed `GlobalUid::ServerVariables` module (https://github.com/zendesk/global_uid/pull/66) +- Removed `options` parameter from `generate_uid` & `generate_many_uids` (https://github.com/zendesk/global_uid/pull/68) ## [3.7.1] - 2020-02-06 ### Added diff --git a/lib/global_uid/active_record_extension.rb b/lib/global_uid/active_record_extension.rb index d5857ae..859d2fa 100644 --- a/lib/global_uid/active_record_extension.rb +++ b/lib/global_uid/active_record_extension.rb @@ -27,12 +27,12 @@ def global_uid_disabled @global_uid_disabled end - def generate_uid(options = {}) - GlobalUid::Base.get_uid_for_class(self, options) + def generate_uid + GlobalUid::Base.get_uid_for_class(self) end - def generate_many_uids(count, options = {}) - GlobalUid::Base.get_many_uids_for_class(self, count, options) + def generate_many_uids(count) + GlobalUid::Base.get_many_uids_for_class(self, count) end def disable_global_uid diff --git a/lib/global_uid/base.rb b/lib/global_uid/base.rb index 5680d3b..1498094 100644 --- a/lib/global_uid/base.rb +++ b/lib/global_uid/base.rb @@ -44,7 +44,7 @@ def self.create_uid_tables(id_table_name, options={}) end end - def self.drop_uid_tables(id_table_name, options={}) + def self.drop_uid_tables(id_table_name) with_connections do |connection| connection.execute("DROP TABLE IF EXISTS `#{id_table_name}`") end @@ -70,7 +70,7 @@ def self.new_connection(name, connection_timeout) end end - def self.init_server_info(options) + def self.init_server_info id_servers = self.global_uid_servers increment_by = self.global_uid_options[:increment_by] @@ -92,12 +92,11 @@ def self.disconnect! self.servers = nil end - def self.setup_connections!(options) + def self.setup_connections! connection_timeout = self.global_uid_options[:connection_timeout] - increment_by = options[:increment_by] if self.servers.nil? - self.servers = init_server_info(options) + self.servers = init_server_info # sorting here sets up each process to have affinity to a particular server. self.servers = self.servers.sort_by { |s| s[:rand] } end @@ -117,9 +116,8 @@ def self.setup_connections!(options) self.servers end - def self.with_connections(options = {}) - options = self.global_uid_options.merge(options) - servers = setup_connections!(options) + def self.with_connections + servers = setup_connections! if !self.global_uid_options[:per_process_affinity] servers = servers.sort_by { rand } #yes, I know it's not true random. @@ -156,11 +154,11 @@ def self.notify(exception, message) end end - def self.get_connections(options = {}) + def self.get_connections with_connections {} end - def self.get_uid_for_class(klass, options = {}) + def self.get_uid_for_class(klass) with_connections do |connection| Timeout.timeout(self.global_uid_options[:query_timeout], TimeoutException) do id = connection.insert("REPLACE INTO #{klass.global_uid_table} (stub) VALUES ('a')") @@ -170,7 +168,7 @@ def self.get_uid_for_class(klass, options = {}) raise NoServersAvailableException, "All global UID servers are gone!" end - def self.get_many_uids_for_class(klass, count, options = {}) + def self.get_many_uids_for_class(klass, count) return [] unless count > 0 with_connections do |connection| Timeout.timeout(self.global_uid_options[:query_timeout], TimeoutException) do diff --git a/lib/global_uid/migration_extension.rb b/lib/global_uid/migration_extension.rb index ced0f62..09bb0dd 100644 --- a/lib/global_uid/migration_extension.rb +++ b/lib/global_uid/migration_extension.rb @@ -25,7 +25,7 @@ def create_table(name, options = {}, &blk) def drop_table(name, options = {}) if !GlobalUid::Base.global_uid_options[:disabled] && options[:use_global_uid] == true id_table_name = options[:global_uid_table] || GlobalUid::Base.id_table_from_name(name) - GlobalUid::Base.drop_uid_tables(id_table_name,options) + GlobalUid::Base.drop_uid_tables(id_table_name) end super(name, options) end