Merge branch '11922-lfs-object-registry-finder' into 'master'

Geo - Remove legacy queries from Geo::LfsObjectRegistryFinder

See merge request gitlab-org/gitlab-ee!14286

ee/app/finders/geo/legacy_lfs_object_registry_finder.rb

-# frozen_string_literal: true
-
-module Geo
-  class LegacyLfsObjectRegistryFinder < RegistryFinder
-    def syncable
-      current_node.lfs_objects.syncable
-    end
-
-    def lfs_objects_synced
-      legacy_inner_join_registry_ids(
-        syncable,
-        Geo::FileRegistry.lfs_objects.synced.pluck_file_key,
-        LfsObject
-      )
-    end
-
-    def lfs_objects_failed
-      legacy_inner_join_registry_ids(
-        syncable,
-        Geo::FileRegistry.lfs_objects.failed.pluck_file_key,
-        LfsObject
-      )
-    end
-
-    def lfs_objects_unsynced(except_file_ids:)
-      registry_file_ids = Geo::FileRegistry.lfs_objects.pluck_file_key | except_file_ids
-
-      legacy_left_outer_join_registry_ids(
-        syncable,
-        registry_file_ids,
-        LfsObject
-      )
-    end
-
-    def lfs_objects_migrated_local(except_file_ids:)
-      legacy_inner_join_registry_ids(
-        current_node.lfs_objects.with_files_stored_remotely,
-        Geo::FileRegistry.lfs_objects.file_id_not_in(except_file_ids).pluck_file_key,
-        LfsObject
-      )
-    end
-
-    def lfs_objects_synced_missing_on_primary
-      legacy_inner_join_registry_ids(
-        syncable,
-        Geo::FileRegistry.lfs_objects.synced.missing_on_primary.pluck_file_key,
-        LfsObject
-      )
-    end
-
-    def registries_for_lfs_objects
-      return Geo::FileRegistry.lfs_objects unless selective_sync?
-
-      legacy_inner_join_registry_ids(
-        Geo::FileRegistry.lfs_objects,
-        current_node.lfs_objects.pluck_primary_key,
-        Geo::FileRegistry,
-        foreign_key: :file_id
-      )
-    end
-  end
-end

ee/app/finders/geo/lfs_object_registry_finder.rb

@@ -2,31 +2,33 @@
 
 module Geo
   class LfsObjectRegistryFinder < FileRegistryFinder
+    def initialize(current_node:)
+      @current_node = Geo::Fdw::GeoNode.find(current_node.id)
+    end
+
+    def count_registry
+      Geo::FileRegistry.lfs_objects.count
+    end
+
     def count_syncable
       syncable.count
     end
 
     def count_synced
-      lfs_objects_synced.count
+      lfs_objects.synced.count
     end
 
     def count_failed
-      lfs_objects_failed.count
+      lfs_objects.failed.count
     end
 
     def count_synced_missing_on_primary
-      lfs_objects_synced_missing_on_primary.count
-    end
-
-    def count_registry
-      Geo::FileRegistry.lfs_objects.count
+      lfs_objects.synced.missing_on_primary.count
     end
 
     def syncable
-      if use_legacy_queries_for_selective_sync?
-        legacy_finder.syncable
-      elsif selective_sync?
-        fdw_geo_node.lfs_objects.syncable
+      if selective_sync?
+        current_node.lfs_objects.syncable
       else
         LfsObject.syncable
       end
@@ -37,34 +39,26 @@ module Geo
     # You can pass a list with `except_file_ids:` so you can exclude items you
     # already scheduled but haven't finished and aren't persisted to the database yet
     #
-    # TODO: Alternative here is to use some sort of window function with a cursor instead
-    #       of simply limiting the query and passing a list of items we don't want
-    #
     # @param [Integer] batch_size used to limit the results returned
     # @param [Array<Integer>] except_file_ids ids that will be ignored from the query
     # rubocop:disable CodeReuse/ActiveRecord
     def find_unsynced(batch_size:, except_file_ids: [])
-      relation =
-        if use_legacy_queries_for_selective_sync?
-          legacy_finder.lfs_objects_unsynced(except_file_ids: except_file_ids)
-        else
-          lfs_objects_unsynced(except_file_ids: except_file_ids)
-        end
-
-      relation.limit(batch_size)
+      current_node
+        .lfs_objects
+        .syncable
+        .missing_file_registry
+        .id_not_in(except_file_ids)
+        .limit(batch_size)
     end
     # rubocop:enable CodeReuse/ActiveRecord
 
     # rubocop:disable CodeReuse/ActiveRecord
     def find_migrated_local(batch_size:, except_file_ids: [])
-      relation =
-        if use_legacy_queries_for_selective_sync?
-          legacy_finder.lfs_objects_migrated_local(except_file_ids: except_file_ids)
-        else
-          lfs_objects_migrated_local(except_file_ids: except_file_ids)
-        end
-
-      relation.limit(batch_size)
+      lfs_objects
+        .inner_join_file_registry
+        .with_files_stored_remotely
+        .id_not_in(except_file_ids)
+        .limit(batch_size)
     end
     # rubocop:enable CodeReuse/ActiveRecord
 
@@ -91,62 +85,12 @@ module Geo
 
     private
 
-    # rubocop:disable CodeReuse/Finder
-    def legacy_finder
-      @legacy_finder ||= Geo::LegacyLfsObjectRegistryFinder.new(current_node: current_node)
-    end
-    # rubocop:enable CodeReuse/Finder
-
-    def fdw_geo_node
-      @fdw_geo_node ||= Geo::Fdw::GeoNode.find(current_node.id)
+    def lfs_objects
+      current_node.lfs_objects
     end
 
     def registries_for_lfs_objects
-      if use_legacy_queries_for_selective_sync?
-        legacy_finder.registries_for_lfs_objects
-      else
-        fdw_geo_node.lfs_object_registries
-      end
-    end
-
-    def lfs_objects_synced
-      if use_legacy_queries_for_selective_sync?
-        legacy_finder.lfs_objects_synced
-      else
-        fdw_geo_node.lfs_objects.synced
-      end
-    end
-
-    def lfs_objects_failed
-      if use_legacy_queries_for_selective_sync?
-        legacy_finder.lfs_objects_failed
-      else
-        fdw_geo_node.lfs_objects.failed
-      end
-    end
-
-    def lfs_objects_unsynced(except_file_ids:)
-      fdw_geo_node
-        .lfs_objects
-        .syncable
-        .missing_file_registry
-        .id_not_in(except_file_ids)
-    end
-
-    def lfs_objects_migrated_local(except_file_ids:)
-      fdw_geo_node
-        .lfs_objects
-        .inner_join_file_registry
-        .with_files_stored_remotely
-        .id_not_in(except_file_ids)
-    end
-
-    def lfs_objects_synced_missing_on_primary
-      if use_legacy_queries_for_selective_sync?
-        legacy_finder.lfs_objects_synced_missing_on_primary
-      else
-        fdw_geo_node.lfs_objects.synced.missing_on_primary
-      end
+      current_node.lfs_object_registries
     end
   end
 end

ee/spec/finders/geo/lfs_object_registry_finder_spec.rb

 require 'spec_helper'
 
-describe Geo::LfsObjectRegistryFinder, :geo do
+describe Geo::LfsObjectRegistryFinder, :geo_fdw do
   include ::EE::GeoHelpers
 
   # Using let() instead of set() because set() does not work properly
@@ -642,5 +642,25 @@ describe Geo::LfsObjectRegistryFinder, :geo do
     end
   end
 
-  it_behaves_like 'a file registry finder'
+  it 'responds to file registry finder methods' do
+    file_registry_finder_methods = %i{
+      syncable
+      count_syncable
+      count_synced
+      count_failed
+      count_synced_missing_on_primary
+      count_registry
+      find_unsynced
+      find_migrated_local
+      find_retryable_failed_registries
+      find_retryable_synced_missing_on_primary_registries
+    }
+
+    file_registry_finder_methods.each do |method|
+      expect(subject).to respond_to(method)
+    end
+  end
+
+  include_examples 'counts all the things'
+  include_examples 'finds all the things'
 end

ee/spec/models/geo_node_status_spec.rb

@@ -1024,7 +1024,7 @@ describe GeoNodeStatus, :geo, :geo_fdw do
 
     context 'on the secondary' do
       it 'calls LfsObjectRegistryFinder#count_syncable' do
-        expect_any_instance_of(Geo::AttachmentRegistryFinder).to receive(:count_syncable)
+        expect_any_instance_of(Geo::LfsObjectRegistryFinder).to receive(:count_syncable)
 
         subject
       end