Expose ActiveJob ability to customize the columns used to order the cursor #859

bbraschi · 2023-07-27T21:09:49Z

JobIteration::ActiveRecordCursor#condition adds an order by clause to the relation returned by the collection method. It default to order on the id column, see JobIteration::ActiveRecordCursor#initialize.

Ordering on id only can degrade performance significantly depending on which indexes are defined.

job-iteration supports configuring which columns are used to order the cursor. This PR explores a way to expose this ability in the maintenance-tasks gem.

bbraschi · 2023-07-27T21:14:20Z

app/models/maintenance_tasks/task.rb

@@ -21,6 +21,9 @@ class NotFoundError < NameError; end
    # @api private
    class_attribute :collection_builder_strategy, default: NullCollectionBuilder.new

+    # As defined by JobIteration::EnumeratorBuilder#build_active_record_enumerator_on_records as `columns`
+    class_attribute :cursor_columns, default: nil


I searched for a way to let subclasses of MaintenanceTasks::Task customize how the enumerator_builder instantiates the underlying relation, without success.

Using a class_attribute as an alternative. Is it appropriate?

This would be overridden by specific tasks, so I think the best thing to do is provide a simple method like collection that can be overridden. A class variable doesn't seem necessary.

TY!

I understand that JobIteration::ActiveRecordBatchEnumerator#initialize here will raise if the collection includes an order by clause and hence that

the collection provided to JobIteration::ActiveRecordBatchEnumerator#initialize cannot be ordered

an explicit set of ordering columns must be given to JobIteration::ActiveRecordBatchEnumerator#initialize

if relation.arel.orders.present? || relation.arel.taken.present? raise JobIteration::ActiveRecordCursor::ConditionNotSupportedError end @base_relation = relation.reorder(@columns.join(","))

MaintenanceTasks::Task could

retrieve the order columns from the relation returned by collection

reorder the the relation returned by collection so it is not ordered anymore

provide the retrieved order columns to the JobIteration::ActiveRecordBatchEnumerator#initialize

However it would make more sense to let JobIteration::ActiveRecordBatchEnumerator itself

support that the collection is ordered and

derive what are the cursor columns.

For active_record_on_records, it seems that we'd also have to make sure that the relation and columns provided to JobIteration::ActiveRecordEnumerator#initialize are consistent.

WDYT?

I misunderstood your comment, got it after rereading more carefully: expose an instance method instead of a class attribute. I find that the class attribute conveys better the intent of customizing the class. Will push a commit to go as you suggest, though.

rafaelfranca · 2023-08-02T16:38:45Z

app/jobs/concerns/maintenance_tasks/task_job_concern.rb

+        enumerator_builder.active_record_on_records(
+          collection,
+          cursor: cursor,
+          **{ columns: @task.class.cursor_columns }.compact,


Why the compact? Isn't better to check if cursor_columns are set?

I used this style because I had not checked in depth how active_record_on_records would behave if passed a nil for the columns argument. Hence I wanted to only provide the columns argument if it was set. I then used compact to avoid an if-then-else.

I have checked that active_record_on_records behaves the same way if the columns argument is absent or nil. It does because JobIteration::ActiveRecordEnumerator#initialize here defaults columns to nil.

-> updated to always provide columns: @task.class.cursor_columns in the Provide columns argument unconditionally to active_record_on_records commit.

rafaelfranca · 2023-08-02T16:40:30Z

app/models/maintenance_tasks/task.rb

@@ -21,6 +21,9 @@ class NotFoundError < NameError; end
    # @api private
    class_attribute :collection_builder_strategy, default: NullCollectionBuilder.new

+    # As defined by JobIteration::EnumeratorBuilder#build_active_record_enumerator_on_records as `columns`


Can you improve this documentation telling what this does instead of linking to method in another library? For the sake of this library, this method is only to configure which column will be used in the Active Record enumerator. Let's explain this and then, we can link to the other library, but if we are going to do that, we should really link to the library documentation, not just mention the reference of the method.

Improvement proposal in Document the cursor_columns attribute inline commit.

rafaelfranca · 2023-08-02T16:41:39Z

test/application_system_test_case.rb

@@ -5,6 +5,7 @@
 require "action_dispatch/system_testing/server"

 ActionDispatch::SystemTesting::Server.silence_puma = true
+Webdrivers::Chromedriver.required_version = "114.0.5735.90"


We can remove this now I believe

TY - Dropped the commit.

gmcgibbon · 2024-01-19T21:01:43Z

@bbraschi are you still interested in working on this?

bbraschi · 2024-01-19T23:03:17Z

@gmcgibbon If you have you have the time to bring this to a conclusion, through this PR or another, please do. Else, I can work on it at the end of Jan 2024.

bbraschi · 2024-01-31T15:19:22Z

app/jobs/concerns/maintenance_tasks/task_job_concern.rb

+        enumerator_builder.active_record_on_records(
+          collection,
+          cursor: cursor,
+          **{ columns: @task.class.cursor_columns }.compact,


I used this style because I had not checked in depth how active_record_on_records would behave if passed a nil for the columns argument. Hence I wanted to only provide the columns argument if it was set. I then used compact to avoid an if-then-else.

I have checked that active_record_on_records behaves the same way if the columns argument is absent or nil. It does because JobIteration::ActiveRecordEnumerator#initialize here defaults columns to nil.

-> updated to always provide columns: @task.class.cursor_columns in the Provide columns argument unconditionally to active_record_on_records commit.

bbraschi · 2024-01-31T15:27:49Z

app/jobs/concerns/maintenance_tasks/task_job_concern.rb

@@ -54,6 +58,7 @@ def build_enumerator(_run, cursor:)
          collection.relation,
          cursor: cursor,
          batch_size: collection.batch_size,
+          **{ columns: @task.class.cursor_columns }.compact,


I have checked that active_record_on_batch_relations behaves the same way if the columns argument is absent or nil. It does because JobIteration::ActiveRecordBatchEnumerator#initialize here defaults columns to nil.

-> updated to always provide columns: @task.class.cursor_columns in the Provide columns argument unconditionally to active_record_on_records commit.

bbraschi · 2024-01-31T16:22:45Z

app/models/maintenance_tasks/task.rb

@@ -21,6 +21,9 @@ class NotFoundError < NameError; end
    # @api private
    class_attribute :collection_builder_strategy, default: NullCollectionBuilder.new

+    # As defined by JobIteration::EnumeratorBuilder#build_active_record_enumerator_on_records as `columns`


Improvement proposal in Document the cursor_columns attribute inline commit.

bbraschi · 2024-02-05T16:54:25Z

@rafaelfranca , @gmcgibbon sorry for the PR getting stalled. Tried to take your comments into account, please review when you can. TY

app/models/maintenance_tasks/task.rb

gmcgibbon · 2024-02-05T17:58:31Z

app/models/maintenance_tasks/task.rb

@@ -21,6 +21,9 @@ class NotFoundError < NameError; end
    # @api private
    class_attribute :collection_builder_strategy, default: NullCollectionBuilder.new

+    # As defined by JobIteration::EnumeratorBuilder#build_active_record_enumerator_on_records as `columns`
+    class_attribute :cursor_columns, default: nil


This would be overridden by specific tasks, so I think the best thing to do is provide a simple method like collection that can be overridden. A class variable doesn't seem necessary.

`job-iteration` supports configuring which columns are used to order the cursor. This commit provides a way to expose this ability in the `maintenance-tasks` gem.

bbraschi force-pushed the expose-cursor_columns branch 2 times, most recently from 459edc3 to e17aba3 Compare July 27, 2023 21:33

bbraschi marked this pull request as ready for review July 27, 2023 21:35

bbraschi requested review from rafaelfranca and buzuloiu July 27, 2023 21:35

bbraschi commented Jul 27, 2023

View reviewed changes

bbraschi force-pushed the expose-cursor_columns branch from e17aba3 to b6057d8 Compare July 27, 2023 21:38

bbraschi self-assigned this Jul 27, 2023

rafaelfranca reviewed Aug 2, 2023

View reviewed changes

bbraschi force-pushed the expose-cursor_columns branch 3 times, most recently from f4e0c5b to 8ec0c18 Compare January 31, 2024 14:23

bbraschi commented Jan 31, 2024

View reviewed changes

bbraschi requested review from rafaelfranca and gmcgibbon January 31, 2024 16:26

gmcgibbon reviewed Feb 5, 2024

View reviewed changes

bbraschi requested a review from gmcgibbon February 6, 2024 03:04

Add cursor_column method to order the cursor

9ff94ee

`job-iteration` supports configuring which columns are used to order the cursor. This commit provides a way to expose this ability in the `maintenance-tasks` gem.

bbraschi force-pushed the expose-cursor_columns branch from 1bbfa40 to 9ff94ee Compare February 8, 2024 17:31

gmcgibbon approved these changes Feb 8, 2024

View reviewed changes

bbraschi merged commit e104de4 into main Feb 8, 2024
41 checks passed

bbraschi deleted the expose-cursor_columns branch February 8, 2024 18:01

nvasilevski mentioned this pull request Feb 9, 2024

Cursor based iteration #852

Closed

shopify-shipit bot temporarily deployed to rubygems February 12, 2024 17:00 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose ActiveJob ability to customize the columns used to order the cursor #859

Expose ActiveJob ability to customize the columns used to order the cursor #859

bbraschi commented Jul 27, 2023 •

edited

Loading

bbraschi Jul 27, 2023

gmcgibbon Feb 5, 2024 •

edited

Loading

bbraschi Feb 5, 2024

bbraschi Feb 5, 2024

bbraschi Feb 6, 2024

rafaelfranca Aug 2, 2023

bbraschi Jan 31, 2024

rafaelfranca Aug 2, 2023

bbraschi Jan 31, 2024

rafaelfranca Aug 2, 2023

bbraschi Jan 31, 2024

gmcgibbon commented Jan 19, 2024

bbraschi commented Jan 19, 2024

bbraschi Jan 31, 2024

bbraschi Jan 31, 2024

bbraschi Jan 31, 2024

bbraschi commented Feb 5, 2024

gmcgibbon Feb 5, 2024 •

edited

Loading

Expose ActiveJob ability to customize the columns used to order the cursor #859

Expose ActiveJob ability to customize the columns used to order the cursor #859

Conversation

bbraschi commented Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

gmcgibbon Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon commented Jan 19, 2024

bbraschi commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbraschi commented Feb 5, 2024

gmcgibbon Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

bbraschi commented Jul 27, 2023 •

edited

Loading

gmcgibbon Feb 5, 2024 •

edited

Loading

gmcgibbon Feb 5, 2024 •

edited

Loading