Addressed review commentsjprovazn-fast-upload-delete

* moved additional methods into FogStorage
* used generic logger
* used single queue for local/fog files

10 files changed, 57 insertions, 48 deletions

diff --git a/app/models/ci/build_trace_chunks/fog.rb b/app/models/ci/build_trace_chunks/fog.rb
index 63d81a093a5..d486e8e292e 100644
--- a/app/models/ci/build_trace_chunks/fog.rb
+++ b/app/models/ci/build_trace_chunks/fog.rb
@@ -5,22 +5,6 @@ module Ci
     class Fog
       include FogStorage
 
-      def data(model)
-        model_key = key(model)
-
-        connection.get_object(bucket_name, key_raw(model_key))[:body]
-      end
-
-      def set_data(model, data)
-        model_key = key(model)
-
-        connection.put_object(bucket_name, key_raw(model_key), data)
-      end
-
-      def delete_data(model)
-        delete_keys([key(model)])
-      end
-
       def keys(relation)
         return [] unless available?
 
diff --git a/app/models/concerns/fog_storage.rb b/app/models/concerns/fog_storage.rb
index 7a45d438a10..6277531c671 100644
--- a/app/models/concerns/fog_storage.rb
+++ b/app/models/concerns/fog_storage.rb
@@ -18,6 +18,22 @@ module FogStorage
     end
   end
 
+  def data(model)
+    model_key = key(model)
+
+    connection.get_object(bucket_name, key_raw(model_key))[:body]
+  end
+
+  def set_data(model, data)
+    model_key = key(model)
+
+    connection.put_object(bucket_name, key_raw(model_key), data)
+  end
+
+  def delete_data(model)
+    delete_keys([key(model)])
+  end
+
   private
 
   def key(model)
diff --git a/app/models/uploads/base.rb b/app/models/uploads/base.rb
new file mode 100644
index 00000000000..92dd94d3836
--- /dev/null
+++ b/app/models/uploads/base.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module Uploads
+  class Base
+    BATCH_SIZE = 100
+
+    attr_reader :logger
+
+    def initialize(logger: nil)
+      @logger ||= Rails.logger
+    end
+  end
+end
diff --git a/app/models/uploads/fog.rb b/app/models/uploads/fog.rb
index 041f395b097..56d41fab26a 100644
--- a/app/models/uploads/fog.rb
+++ b/app/models/uploads/fog.rb
@@ -1,11 +1,9 @@
 # frozen_string_literal: true
 
 module Uploads
-  class Fog
+  class Fog < Base
     include FogStorage
 
-    BATCH_SIZE = 100
-
     def keys(relation)
       return [] unless available?
 
@@ -14,7 +12,7 @@ module Uploads
 
     def delete_keys_async(keys_to_delete)
       keys_to_delete.each_slice(BATCH_SIZE) do |batch|
-        UploadDeleteFogWorker.perform_async(batch)
+        UploadDeleteWorker.perform_async(self.class, batch)
       end
     end
 
diff --git a/app/models/uploads/local.rb b/app/models/uploads/local.rb
index bc1c396347e..43512911a85 100644
--- a/app/models/uploads/local.rb
+++ b/app/models/uploads/local.rb
@@ -1,9 +1,7 @@
 # frozen_string_literal: true
 
 module Uploads
-  class Local
-    BATCH_SIZE = 100
-
+  class Local < Base
     def keys(relation)
       relation.includes(:model).find_each.map {|u| u.absolute_path }
     end
@@ -16,7 +14,7 @@ module Uploads
 
     def delete_keys_async(keys_to_delete)
       keys_to_delete.each_slice(BATCH_SIZE) do |batch|
-        UploadDeleteLocalWorker.perform_async(batch)
+        UploadDeleteWorker.perform_async(self.class, batch)
       end
     end
 
@@ -24,12 +22,12 @@ module Uploads
 
     def delete_file(path)
       unless exists?(path)
-        Sidekiq.logger.warn("File '#{path}' doesn't exist, skipping")
+        logger.warn("File '#{path}' doesn't exist, skipping")
         return
       end
 
       unless in_uploads?(path)
-        Sidekiq.logger.warn("Path '#{path}' is not in uploads dir, skipping")
+        logger.warn("Path '#{path}' is not in uploads dir, skipping")
         return
       end
 
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 53e474eb181..28a4871f295 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -132,5 +132,4 @@
 - create_note_diff_file
 - delete_diff_files
 - detect_repository_languages
-- upload_delete_fog
-- upload_delete_local
+- upload_delete
diff --git a/app/workers/upload_delete_fog_worker.rb b/app/workers/upload_delete_fog_worker.rb
deleted file mode 100644
index 1e0e69c6aac..00000000000
--- a/app/workers/upload_delete_fog_worker.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-# frozen_string_literal: true
-
-class UploadDeleteFogWorker
-  include ApplicationWorker
-
-  def perform(keys)
-    Uploads::Fog.new.delete_keys(keys)
-  end
-end
diff --git a/app/workers/upload_delete_local_worker.rb b/app/workers/upload_delete_local_worker.rb
deleted file mode 100644
index b11644be926..00000000000
--- a/app/workers/upload_delete_local_worker.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-# frozen_string_literal: true
-
-class UploadDeleteLocalWorker
-  include ApplicationWorker
-
-  def perform(keys)
-    Uploads::Local.new.delete_keys(keys)
-  end
-end
diff --git a/app/workers/upload_delete_worker.rb b/app/workers/upload_delete_worker.rb
new file mode 100644
index 00000000000..3b1a3a2b58c
--- /dev/null
+++ b/app/workers/upload_delete_worker.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class UploadDeleteWorker
+  include ApplicationWorker
+
+  def perform(klass, keys)
+    klass = begin
+      Kernel.const_get(class_name)
+    rescue NameError
+      nil
+    end
+
+    unless klass
+      logger.error "Unknown class '#{klass}'"
+      return
+    end
+
+    klass.new(logger: logger).delete_keys(keys)
+  end
+end
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 7634a8cc94c..766ab2d17fb 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -81,5 +81,4 @@
   - [delete_diff_files, 1]
   - [detect_repository_languages, 1]
   - [auto_devops, 2]
-  - [upload_delete_fog, 1]
-  - [upload_delete_local, 1]
+  - [upload_delete, 1]