GEO: Address review comments

app/models/geo/file_registry.rb

 class Geo::FileRegistry < Geo::BaseRegistry
   scope :failed, -> { where(success: false) }
   scope :synced, -> { where(success: true) }
-  scope :to_be_retried, -> { where('retry_at is NULL OR retry_at < ?', Time.now) }
+  scope :retry_due, -> { where('retry_at is NULL OR retry_at < ?', Time.now) }
   scope :lfs_objects, -> { where(file_type: :lfs) }
   scope :attachments, -> { where(file_type: Geo::FileService::DEFAULT_OBJECT_TYPES) }
 end

app/models/geo/project_registry.rb

@@ -15,7 +15,7 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
     where(repository_sync_failed.or(wiki_sync_failed))
   end
 
-  def self.to_be_retried
+  def self.retry_due
     where(
       arel_table[:repository_retry_at].lt(Time.now)
         .or(arel_table[:wiki_retry_at].lt(Time.now))

app/services/geo/base_sync_service.rb

@@ -172,7 +172,12 @@ module Geo
     end
 
     def disk_path_temp
-      "#{repository.disk_path}_temp"
+      unless @disk_path_temp
+        random_string = (0...6).map { ('a'..'z').to_a.sample }.join
+        @disk_path_temp = "#{repository.disk_path}_#{random_string}"
+      end
+
+      @disk_path_temp
     end
 
     def build_temporary_repository

app/services/geo/repository_sync_service.rb

@@ -17,7 +17,6 @@ module Geo
 
       if redownload
         log_info('Redownloading repository')
-        clean_up_temporary_repository
         fetch_geo_mirror(build_temporary_repository)
         set_temp_repository_as_main
       else
@@ -37,6 +36,8 @@ module Geo
       log_error('Invalid repository', e)
       registry.update(force_to_redownload_repository: true)
       expire_repository_caches
+    ensure
+      clean_up_temporary_repository if redownload
     end
 
     def expire_repository_caches

app/services/geo/wiki_sync_service.rb

@@ -16,7 +16,6 @@ module Geo
 
       if redownload
         log_info('Redownloading wiki')
-        clean_up_temporary_repository
         fetch_geo_mirror(build_temporary_repository)
         set_temp_repository_as_main
       else
@@ -37,6 +36,8 @@ module Geo
     rescue Gitlab::Git::Repository::NoRepository => e
       log_error('Invalid wiki', e)
       registry.update(force_to_redownload_wiki: true)
+    ensure
+      clean_up_temporary_repository if redownload
     end
 
     def ssh_url_to_wiki

app/workers/geo/file_download_dispatch_worker.rb

@@ -33,7 +33,7 @@ module Geo
     def find_failed_objects(batch_size:)
       Geo::FileRegistry
         .failed
-        .to_be_retried
+        .retry_due
         .limit(batch_size)
         .pluck(:file_id, :file_type)
     end

app/workers/geo/repository_sync_worker.rb

@@ -33,7 +33,7 @@ module Geo
     def find_project_ids_updated_recently(batch_size:)
       current_node.project_registries
                   .dirty
-                  .to_be_retried
+                  .retry_due
                   .order(Gitlab::Database.nulls_first_order(:last_repository_synced_at, :desc))
                   .limit(batch_size)
                   .pluck(:project_id)

db/geo/migrate/20171101105200_add_retry_count_fields_to_registries.rb

 class AddRetryCountFieldsToRegistries < ActiveRecord::Migration
-  def change
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
     add_column :file_registry, :retry_count, :integer
     add_column :file_registry, :retry_at, :datetime
 
@@ -10,5 +16,23 @@ class AddRetryCountFieldsToRegistries < ActiveRecord::Migration
     add_column :project_registry, :wiki_retry_count, :integer
     add_column :project_registry, :wiki_retry_at, :datetime
     add_column :project_registry, :force_to_redownload_wiki, :boolean
+
+    # Indecies
+    add_concurrent_index :file_registry, :retry_at
+    add_concurrent_index :project_registry, :repository_retry_at
+    add_concurrent_index :project_registry, :wiki_retry_at
+  end
+
+  def down
+    remove_column :file_registry, :retry_count, :integer
+    remove_column :file_registry, :retry_at, :datetime
+
+    remove_column :project_registry, :repository_retry_count, :integer
+    remove_column :project_registry, :repository_retry_at, :datetime
+    remove_column :project_registry, :force_to_redownload_repository, :boolean
+
+    remove_column :project_registry, :wiki_retry_count, :integer
+    remove_column :project_registry, :wiki_retry_at, :datetime
+    remove_column :project_registry, :force_to_redownload_wiki, :boolean
   end
 end

db/geo/schema.rb

@@ -32,6 +32,7 @@ ActiveRecord::Schema.define(version: 20171101105200) do
 
   add_index "file_registry", ["file_type", "file_id"], name: "index_file_registry_on_file_type_and_file_id", unique: true, using: :btree
   add_index "file_registry", ["file_type"], name: "index_file_registry_on_file_type", using: :btree
+  add_index "file_registry", ["retry_at"], name: "index_file_registry_on_retry_at", using: :btree
   add_index "file_registry", ["success"], name: "index_file_registry_on_success", using: :btree
 
   create_table "project_registry", force: :cascade do |t|
@@ -54,7 +55,9 @@ ActiveRecord::Schema.define(version: 20171101105200) do
   add_index "project_registry", ["last_repository_successful_sync_at"], name: "index_project_registry_on_last_repository_successful_sync_at", using: :btree
   add_index "project_registry", ["last_repository_synced_at"], name: "index_project_registry_on_last_repository_synced_at", using: :btree
   add_index "project_registry", ["project_id"], name: "index_project_registry_on_project_id", unique: true, using: :btree
+  add_index "project_registry", ["repository_retry_at"], name: "index_project_registry_on_repository_retry_at", using: :btree
   add_index "project_registry", ["resync_repository"], name: "index_project_registry_on_resync_repository", using: :btree
   add_index "project_registry", ["resync_wiki"], name: "index_project_registry_on_resync_wiki", using: :btree
+  add_index "project_registry", ["wiki_retry_at"], name: "index_project_registry_on_wiki_retry_at", using: :btree
 
 end

spec/services/geo/base_sync_service_spec.rb

@@ -9,10 +9,10 @@ describe Geo::BaseSyncService do
 
   describe '#lease_key' do
     it 'returns a key in the correct pattern' do
-      described_class.type = :test
+      allow(described_class).to receive(:type) { :wiki }
       allow(project).to receive(:id) { 999 }
 
-      expect(subject.lease_key).to eq('geo_sync_service:test:999')
+      expect(subject.lease_key).to eq('geo_sync_service:wiki:999')
     end
   end