Merge pull request #1041 from dmlond/DDS-922_optimize_child_deletion_job

DDS-922_optimize_child_deletion_job

Author: dmann

app/jobs/child_deletion_job.rb

@@ -1,12 +1,10 @@
 class ChildDeletionJob < ApplicationJob
   queue_as :child_deletion
 
-  def perform(job_transaction, parent)
+  def perform(job_transaction, parent, page)
     self.class.start_job(job_transaction)
-    parent.children.each do |child|
-      child.current_transaction = job_transaction if child.class.include? ChildMinder
-      child.update(is_deleted: true)
-    end
+    parent.current_transaction = job_transaction
+    parent.delete_children(page)
     self.class.complete_job(job_transaction)
   end
 end

app/models/concerns/child_minder.rb

@@ -9,20 +9,30 @@ def manage_children
     newly_deleted = is_deleted_changed? && is_deleted?
     yield
     if has_children? && newly_deleted
-      ChildDeletionJob.perform_later(
-        ChildDeletionJob.initialize_job(self),
-        self
-      )
+      (1..paginated_children.total_pages).each do |page|
+        ChildDeletionJob.perform_later(
+          ChildDeletionJob.initialize_job(self),
+          self,
+          page
+        )
+      end
     end
   end
 
-  def delete_children
-    children.each do |child|
+  def has_children?
+    children.count > 0
+  end
+
+  def delete_children(page)
+    paginated_children(page).each do |child|
+      child.current_transaction = current_transaction if child.class.include? ChildMinder
       child.update(is_deleted: true)
     end
   end
 
-  def has_children?
-    children.count > 0
+  private
+
+  def paginated_children(page=1)
+    children.page(page).per(Rails.application.config.max_children_per_job)
   end
 end

config/initializers/child_deletion_job.rb

@@ -0,0 +1 @@
+Rails.application.config.max_children_per_job = ENV['MAX_CHILDREN_PER_JOB'] ? ENV['MAX_CHILDREN_PER_JOB'].to_i : 100

spec/jobs/child_deletion_job_spec.rb

@@ -1,6 +1,48 @@
 require 'rails_helper'
 
 RSpec.describe ChildDeletionJob, type: :job do
+
+  shared_examples 'a ChildDeletionJob' do |
+      parent_sym,
+      child_folder_sym,
+      child_file_sym
+    |
+    let(:parent) { send(parent_sym) }
+    let(:job_transaction) { described_class.initialize_job(parent) }
+    let(:child_folder) { send(child_folder_sym) }
+    let(:child_file) { send(child_file_sym) }
+    let(:prefix) { Rails.application.config.active_job.queue_name_prefix }
+    let(:prefix_delimiter) { Rails.application.config.active_job.queue_name_delimiter }
+    include_context 'with job runner', described_class
+
+    it { is_expected.to be_an ApplicationJob }
+    it { expect(prefix).not_to be_nil }
+    it { expect(prefix_delimiter).not_to be_nil }
+    it { expect(described_class.queue_name).to eq("#{prefix}#{prefix_delimiter}child_deletion") }
+    it {
+      expect {
+        described_class.perform_now
+      }.to raise_error(ArgumentError)
+      expect {
+        described_class.perform_now(job_transaction)
+      }.to raise_error(ArgumentError)
+      expect {
+        described_class.perform_now(job_transaction, parent)
+      }.to raise_error(ArgumentError)
+    }
+
+    context 'perform_now' do
+      let(:page) { 1 }
+
+      it {
+        expect(child_folder).to be_persisted
+        expect(child_file).to be_persisted
+        expect(parent).to receive(:delete_children).with(page)
+        described_class.perform_now(job_transaction, parent, page)
+      }
+    end
+  end
+
   before do
     expect(folder_child).to be_persisted
   end

spec/support/concerns/child_minder_shared_examples.rb

@@ -65,6 +65,8 @@
           ChildDeletionJob.initialize_job(subject)
         }
         before do
+          @old_max = Rails.application.config.max_children_per_job
+          Rails.application.config.max_children_per_job = 1
           expect(child_folder).to be_persisted
           expect(child_folder.is_deleted?).to be_falsey
           expect(valid_child_file).to be_persisted
@@ -73,13 +75,19 @@
           expect(invalid_child_file.is_deleted?).to be_falsey
         end
 
+        after do
+          Rails.application.config.max_children_per_job = @old_max
+        end
+
         it {
           expect(subject).to be_has_children
           subject.is_deleted = true
           yield_called = false
-          expect(ChildDeletionJob).to receive(:initialize_job)
-            .with(subject).and_return(job_transaction)
-          expect(ChildDeletionJob).to receive(:perform_later).with(job_transaction, subject)
+          expect(ChildDeletionJob).to receive(:initialize_job).exactly(subject.children.count)
+            .with(subject).times.and_return(job_transaction)
+          (1..subject.children.count).each do |page|
+            expect(ChildDeletionJob).to receive(:perform_later).with(job_transaction, subject, page)
+          end
           subject.manage_children do
             yield_called = true
           end
@@ -133,23 +141,41 @@
   end
 
   describe '#delete_children' do
-    it {
-      is_expected.to respond_to(:delete_children)
-    }
-    it {
-      expect(child_folder).to be_persisted
-      expect(child_folder.is_deleted?).to be_falsey
-      expect(valid_child_file).to be_persisted
-      expect(valid_child_file.is_deleted?).to be_falsey
-      expect(invalid_child_file).to be_persisted
-      expect(invalid_child_file.is_deleted?).to be_falsey
-      subject.delete_children
-      expect(child_folder.reload).to be_truthy
-      expect(child_folder.is_deleted?).to be_truthy
-      valid_child_file.reload
-      expect(valid_child_file.is_deleted?).to be_truthy
-      invalid_child_file.reload
-      expect(invalid_child_file.is_deleted?).to be_truthy
-    }
+    it { is_expected.not_to respond_to(:delete_children).with(0).arguments }
+    it { is_expected.to respond_to(:delete_children).with(1).argument }
+
+    context 'called', :vcr do
+      let(:job_transaction) { ChildDeletionJob.initialize_job(subject) }
+      let(:child_job_transaction) { ChildDeletionJob.initialize_job(child_folder) }
+      let(:child_folder_file) { FactoryGirl.create(:data_file, parent: child_folder)}
+      let(:page) { 1 }
+
+      before do
+        expect(child_folder).to be_persisted
+        expect(child_folder_file).to be_persisted
+        expect(valid_child_file.is_deleted?).to be_falsey
+        @old_max = Rails.application.config.max_children_per_job
+        Rails.application.config.max_children_per_job = subject.children.count + child_folder.children.count
+      end
+
+      after do
+        Rails.application.config.max_children_per_job = @old_max
+      end
+
+      it {
+        subject.current_transaction = job_transaction
+        expect(ChildDeletionJob).to receive(:initialize_job)
+          .with(child_folder)
+          .and_return(child_job_transaction)
+        expect(ChildDeletionJob).to receive(:perform_later)
+          .with(child_job_transaction, child_folder, page).and_call_original
+        subject.delete_children(page)
+        expect(child_folder.reload).to be_truthy
+        expect(child_folder.is_deleted?).to be_truthy
+        expect(valid_child_file.reload).to be_truthy
+        expect(valid_child_file.is_deleted?).to be_truthy
+      }
+    end
+
   end
-end
+ end

spec/support/job_shared_examples.rb

@@ -163,52 +163,3 @@
       .and_call_original
   end
 end
-
-shared_examples 'a ChildDeletionJob' do |
-    parent_sym,
-    child_folder_sym,
-    child_file_sym
-  |
-  let(:parent) { send(parent_sym) }
-  let(:job_transaction) { described_class.initialize_job(parent) }
-  let(:child_folder) { send(child_folder_sym) }
-  let(:child_file) { send(child_file_sym) }
-  let(:prefix) { Rails.application.config.active_job.queue_name_prefix }
-  let(:prefix_delimiter) { Rails.application.config.active_job.queue_name_delimiter }
-  include_context 'with job runner', described_class
-
-  it { is_expected.to be_an ApplicationJob }
-  it { expect(prefix).not_to be_nil }
-  it { expect(prefix_delimiter).not_to be_nil }
-  it { expect(described_class.queue_name).to eq("#{prefix}#{prefix_delimiter}child_deletion") }
-  it {
-    expect {
-      described_class.perform_now
-    }.to raise_error(ArgumentError)
-    expect {
-      described_class.perform_now(parent)
-    }.to raise_error(ArgumentError)
-  }
-
-  context 'perform_now', :vcr do
-    let(:child_job_transaction) { described_class.initialize_job(child_folder) }
-    include_context 'tracking job', :job_transaction
-
-    it {
-      expect(child_folder).to be_persisted
-      expect(child_file.is_deleted?).to be_falsey
-
-      expect(described_class).to receive(:initialize_job)
-        .with(child_folder)
-        .and_return(child_job_transaction)
-      expect(described_class).to receive(:perform_later)
-        .with(child_job_transaction, child_folder).and_call_original
-
-      described_class.perform_now(job_transaction, parent)
-      expect(child_folder.reload).to be_truthy
-      expect(child_folder.is_deleted?).to be_truthy
-      expect(child_file.reload).to be_truthy
-      expect(child_file.is_deleted?).to be_truthy
-    }
-  end
-end