Major refactor of code for cleanliness, added a flag to disable proxy…

…sql tagging, fixed very slow tests

CHANGELOG.md

@@ -1,3 +1,13 @@
+# 3.5.4 (Dec, 2021)
+* Refactored the way options are handled internally. Code is now much clearer to understand
+* Removed optional connection_options from `Lhm.setup` and `Lhm.connection`
+* Option `reconnect_with_consistent_host` will now be provided with `options` for `Lhm.change_table`
+* Option `disable_proxysql_tags` introduced as part of `options` for `Lhm.change_table`. This will deactivate 
+  the internal proxysql tagging of queries and will use the ones provided by ActiveRecord.
+
+# 3.5.3 (Dec, 2021)
+* Adds ProxySQL comments at the end of query to accommodate for internal tool's requirements
+
 # 3.5.2 (Dec, 2021)
 * Fixed error on undefined connection, when calling `Lhm.connection` without calling `Lhm.setup` first
 * Changed `Lhm.connection.connection` to `lhm.connection.ar_connection` for increased clarity and readability

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    lhm-shopify (3.5.3)
+    lhm-shopify (3.5.4)
       retriable (>= 3.0.0)
 
 GEM

README.md

@@ -113,8 +113,8 @@ tables must be cleaned up.
 LHM can recover from connection loss. However, when used in conjunction with ProxySQL, there are multiple ways that
 connection loss could induce data loss (if triggered by a failover). Therefore  it will perform additional checks to
 ensure that the MySQL host stays consistent across the schema migrations if the feature is enabled. 
-This is done by tagging every query with `/*maintenance:lhm*/`, which will be recognized by ProxySQL. 
-However, to get this feature working, a new ProxySQL query rule must be added.
+This is done by tagging every query with `/*maintenance:lhm*/`, which will be recognized by ProxySQL (these tags 
+can be disabled).  However, to get this feature working, a new ProxySQL query rule must be added.
 ```cnf
 {
   rule_id = <rule id>
@@ -145,12 +145,21 @@ forwarded to the right target.
 ```
 
 Once these changes are added to the ProxySQL configuration (either through `.cnf` or dynamically through the admin interface), 
-the feature can be enabled. This is done by adding this flag when doing the initial setup:
+the feature can be enabled. This is done by adding this flag when providing options to the migration:
 ```ruby
- Lhm.setup(connection, options: {reconnect_with_consistent_host: true})
+ Lhm.change_table(..., options: {reconnect_with_consistent_host: true}) do |t|
+  ...
+end
 ```
 **Note**: This feature is disabled by default
 
+The annotations mentioned above can also be disabled if ActiveRecord's QueryLogs or Marginalia are enabled . This is done by providing
+the following option to `Lhm.change_table`. 
+```ruby
+ Lhm.change_table(..., options: {disable_proxysql_tags: true}) do |t|
+  ...
+end
+```
 ## Throttler
 
 LHM uses a throttling mechanism to read data in your original table. By default, 2,000 rows are read each 0.1 second. If you want to change that behaviour, you can pass an instance of a throttler with the `throttler` option. In this example, 1,000 rows will be read with a 10 second delay between each processing:

Rakefile

@@ -16,15 +16,16 @@ Rake::TestTask.new('integration') do |t|
   t.libs << 'spec'
   t.test_files = FileList['spec/integration/**/*_spec.rb']
   t.verbose = true
-  end
+end
 
 Rake::TestTask.new('dev') do |t|
   t.libs << 'lib'
   t.libs << 'spec'
-  t.test_files = FileList[
-    'spec/test_helper.rb',
-  #  Add file to test individually
-  ]
+
+  files = FileList.new('spec/test_helper.rb')
+  files.add(ENV["SINGLE_TEST"]) if ENV["SINGLE_TEST"]
+  t.test_files = files
+
   t.verbose = true
 end
 

dev.yml

@@ -5,7 +5,7 @@ up:
           or: [[email protected]]
           conflicts: [shopify/shopify/mysql-client, mysql-connector-c, mysql, mysql-client]
       - wget
-  - ruby: 2.7.0
+  - ruby: 2.7.5
   - bundler
   - custom:
       name: Get Appraisal gems
@@ -31,7 +31,7 @@ commands:
       if [[ $# -eq 0 ]]; then
         bundle exec rake unit && bundle exec rake integration
       else
-        bundle exec rake dev TEST="$@"
+        SINGLE_TEST="$@" bundle exec rake dev
       fi
   appraisals: bundle exec appraisal rake specs
   cov: rm -rf coverage; COV=1 bundle exec rake unit && bundle exec rake integration; open coverage/index.html

gemfiles/activerecord_5.2.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    lhm-shopify (3.5.3)
+    lhm-shopify (3.5.4)
       retriable (>= 3.0.0)
 
 GEM

gemfiles/activerecord_6.0.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    lhm-shopify (3.5.3)
+    lhm-shopify (3.5.4)
       retriable (>= 3.0.0)
 
 GEM

gemfiles/activerecord_6.1.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    lhm-shopify (3.5.3)
+    lhm-shopify (3.5.4)
       retriable (>= 3.0.0)
 
 GEM

gemfiles/activerecord_7.0.0.alpha2.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    lhm-shopify (3.5.3)
+    lhm-shopify (3.5.4)
       retriable (>= 3.0.0)
 
 GEM

lib/lhm.rb

@@ -46,15 +46,21 @@ module Lhm
   #   Use atomic switch to rename tables (defaults to: true)
   #   If using a version of mysql affected by atomic switch bug, LHM forces user
   #   to set this option (see SqlHelper#supports_atomic_switch?)
+  # @option options [Boolean] :reconnect_with_consistent_host
+  #   Active / Deactivate ProxySQL-aware reconnection procedure (default to: false)
+  # @option options [Boolean] :disable_proxysql_tags
+  #   Active / Deactivate ProxySQL tags to be added to queries (if used with Marginalia or ActiveRecord QueryLogs)
   # @yield [Migrator] Yielded Migrator object records the changes
   # @return [Boolean] Returns true if the migration finishes
   # @raise [Error] Raises Lhm::Error in case of a error and aborts the migration
   def change_table(table_name, options = {}, &block)
-    origin = Table.parse(table_name, connection)
-    invoker = Invoker.new(origin, connection)
-    block.call(invoker.migrator)
-    invoker.run(options)
-    true
+    with_flags(options) do
+      origin = Table.parse(table_name, connection)
+      invoker = Invoker.new(origin, connection)
+      block.call(invoker.migrator)
+      invoker.run(options)
+      true
+    end
   end
 
   # Cleanup tables and triggers
@@ -86,27 +92,16 @@ def cleanup_current_run(run, table_name, options = {})
   # Setups DB connection
   #
   # @param [ActiveRecord::Base] connection ActiveRecord Connection
-  # @param [Hash] connection_options Optional options (defaults to: empty hash)
-  # @option connection_options [Boolean] :reconnect_with_consistent_host
-  #   Active / Deactivate ProxySQL-aware reconnection procedure (default to: false)
-  def setup(connection, connection_options = {})
-    @@connection = Connection.new(connection: connection, options: connection_options)
+  def setup(connection)
+    @@connection = Connection.new(connection: connection)
   end
 
-  # Setups DB connection
-  #
-  # @param [Hash] connection_options Optional options (defaults to: empty hash)
-  # @option connection_options [Boolean] :reconnect_with_consistent_host
-  #   Active / Deactivate ProxySQL-aware reconnection procedure (default to: false)
-  def connection(connection_options = nil)
+  # Returns DB connection (or initializes it if not created yet)
+  def connection
     @@connection ||= begin
-      raise 'Please call Lhm.setup' unless defined?(ActiveRecord)
-      @@connection = Connection.new(connection: ActiveRecord::Base.connection, options: connection_options || {})
-    end
-
-    @@connection.process_connection_options(connection_options) unless connection_options.nil?
-
-    @@connection
+                       raise 'Please call Lhm.setup' unless defined?(ActiveRecord)
+                       @@connection = Connection.new(connection: ActiveRecord::Base.connection)
+                     end
   end
 
   def self.logger=(new_logger)
@@ -148,4 +143,19 @@ def drop_tables_and_triggers(run = false, triggers, tables)
       false
     end
   end
+
+  def with_flags(options)
+    old_flags = {
+      reconnect_with_consistent_host: Lhm.connection.reconnect_with_consistent_host,
+      disable_proxysql_tags: ProxySQLHelper.disable_tags
+    }
+
+    Lhm.connection.reconnect_with_consistent_host = options[:reconnect_with_consistent_host] || false
+    ProxySQLHelper.disable_tags = options[:disable_proxysql_tags] || false
+
+    yield
+  ensure
+    Lhm.connection.reconnect_with_consistent_host = old_flags[:reconnect_with_consistent_host]
+    ProxySQLHelper.disable_tags = old_flags[:disable_proxysql_tags]
+  end
 end

lib/lhm/atomic_switcher.rb

@@ -16,12 +16,13 @@ class AtomicSwitcher
 
     attr_reader :connection
 
-    def initialize(migration, connection = nil, options={})
+    LOG_PREFIX = "AtomicSwitcher"
+
+    def initialize(migration, connection = nil)
       @migration = migration
       @connection = connection
       @origin = migration.origin
       @destination = migration.destination
-      @retry_options = options[:retriable] || {}
     end
 
     def atomic_switch
@@ -39,7 +40,7 @@ def validate
     private
 
     def execute
-      @connection.execute(atomic_switch, should_retry: true, retry_options: @retry_options)
+      @connection.execute(atomic_switch, should_retry: true, log_prefix: LOG_PREFIX)
     end
   end
 end

lib/lhm/chunk_insert.rb

@@ -3,6 +3,9 @@
 
 module Lhm
   class ChunkInsert
+
+    LOG_PREFIX = "ChunkInsert"
+
     def initialize(migration, connection, lowest, highest, retry_options = {})
       @migration = migration
       @connection = connection
@@ -12,7 +15,7 @@ def initialize(migration, connection, lowest, highest, retry_options = {})
     end
 
     def insert_and_return_count_of_rows_created
-      @connection.update(sql, should_retry: true, retry_options: @retry_options)
+      @connection.update(sql, should_retry: true, log_prefix: LOG_PREFIX)
     end
 
     def sql

lib/lhm/chunker.rb

@@ -13,6 +13,8 @@ class Chunker
 
     attr_reader :connection
 
+    LOG_PREFIX = "Chunker"
+
     # Copy from origin to destination in chunks of size `stride`.
     # Use the `throttler` class to sleep between each stride.
     def initialize(migration, connection = nil, options = {})
@@ -31,9 +33,7 @@ def initialize(migration, connection = nil, options = {})
       @retry_options = options[:retriable] || {}
       @retry_helper = SqlRetry.new(
         @connection,
-        retry_options: {
-          log_prefix: "Chunker"
-        }.merge!(@retry_options)
+        retry_options: @retry_options
       )
     end
 
@@ -79,7 +79,7 @@ def execute
     private
 
     def raise_on_non_pk_duplicate_warning
-      @connection.execute("show warnings", should_retry: true, retry_options: @retry_options).each do |level, code, message|
+      @connection.execute("show warnings", should_retry: true, log_prefix: LOG_PREFIX).each do |level, code, message|
         unless message.match?(/Duplicate entry .+ for key 'PRIMARY'/)
           m = "Unexpected warning found for inserted row: #{message}"
           Lhm.logger.warn(m)
@@ -94,14 +94,14 @@ def bottom
 
     def verify_can_run
       return unless @verifier
-      @retry_helper.with_retries(@retry_options) do |retriable_connection|
+      @retry_helper.with_retries(log_prefix: LOG_PREFIX) do |retriable_connection|
         raise "Verification failed, aborting early" if !@verifier.call(retriable_connection)
       end
     end
 
     def upper_id(next_id, stride)
       sql = "select id from `#{ @migration.origin_name }` where id >= #{ next_id } order by id limit 1 offset #{ stride - 1}"
-      top = @connection.select_value(sql, should_retry: true, retry_options: @retry_options)
+      top = @connection.select_value(sql, should_retry: true, log_prefix: LOG_PREFIX)
 
       [top ? top.to_i : @limit, @limit].min
     end

lib/lhm/cleanup/current.rb

@@ -4,6 +4,9 @@
 module Lhm
   module Cleanup
     class Current
+
+      LOG_PREFIX = "Current"
+
       def initialize(run, origin_table_name, connection, options={})
         @run = run
         @table_name = TableName.new(origin_table_name)
@@ -54,7 +57,7 @@ def lhmn_tables_for_origin
 
       def execute_ddls
         ddls.each do |ddl|
-            @connection.execute(ddl, should_retry: true, retry_options: @retry_config)
+            @connection.execute(ddl, should_retry: true, log_prefix: LOG_PREFIX)
         end
         Lhm.logger.info("Dropped triggers on #{@lhm_triggers_for_origin.join(', ')}")
         Lhm.logger.info("Dropped tables #{@lhm_triggers_for_origin.join(', ')}")