CCOL-2440: Add consumer configuration to save associated keys prior to saving primary record #217

lionelpereira · 2024-05-07T21:12:35Z

Pull Request Template

Description

Normal bulk consumption logic:

Widget:
has_one : detail

Record class records (Widget) are saved
Record class primary keys are filled from DB
Associated records (Details) are updated with foreign key from record class

Backfill consumption logic (this change)

FidgetWidget
belongs_to: widget
belong_to: fidget

Notes:

Record class cannot be upserted first as foreign_keys widget_id, fidget_id are required

Associated records (Widget & Fidget) are created and linked to record class (FidgetWidget)
Associated records are saved
Primary keys (widget_id, fidget_id) of associated records are filled from DB
Foreign keys of record class are filled with newly saved primary keys of associated records

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A
Test B

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added a line in the CHANGELOG describing this change, under the UNRELEASED heading
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

dorner · 2024-05-08T13:28:52Z

@lionelpereira can you provide some more information in this PR as to what the use case is here and why it isn't covered by existing functionality?

lionelpereira · 2024-05-08T15:10:26Z

@lionelpereira can you provide some more information in this PR as to what the use case is here and why it isn't covered by existing functionality?

@dorner Updated the PR description

dorner · 2024-05-08T17:40:30Z

lib/deimos/config/configuration.rb

@@ -476,6 +477,8 @@ def self.configure_producer_or_consumer(kafka_config)
      # @return [Block]
      setting :bulk_import_id_generator, nil

+      setting :backfill_associations, false


YARD comment please.

I'm not sure I like the naming of this... You're actually backfilling the main record with the association IDs. Maybe something as simple as save_associations_first?

dorner · 2024-05-08T17:41:38Z

lib/deimos/active_record_consume/mass_updater.rb

+        associations = {}
+        record_list.associations.each do |assoc|
+          col = @bulk_import_id_column if assoc.klass.column_names.include?(@bulk_import_id_column)
+          associations[[assoc.name, assoc.klass, col, assoc.foreign_key]] = []


Feels like you should be able to just use [assoc, col] and reference what you need out of the association further down?

Also you don't need to do this up-front - just change line 93 to associations = Hash.new([]) which will put an empty array as the default for any key instead of nil.

dorner · 2024-05-08T17:42:38Z

lib/deimos/active_record_consume/mass_updater.rb

+            # Associate this associated batch record's record with the primary record to
+            # retrieve foreign_key after associated records have been saved and primary
+            # keys have been filled
+            primary_batch_record.record.send(:"#{assoc}=", batch_record.record)


Pretty sure you can do primary_batch_record.record[assoc] = batch_record.record

I was getting a NoMethodError when I tried this

dorner · 2024-05-08T17:44:36Z

spec/active_record_consume/mass_updater_spec.rb

+            case klass.to_s
+            when 'Widget', 'Fidget'
+              %w(id)
+            when 'WidgetFidgets'


Why is this the only one that's plural?

dorner · 2024-05-09T13:31:24Z

lib/deimos/active_record_consume/mass_updater.rb

+        record_list.associations.each do |assoc|
+          col = @bulk_import_id_column if assoc.klass.column_names.include?(@bulk_import_id_column)
+          associations[[assoc, col]] = []
+        end


You should be able to remove lines 95-98.

I would still need these lines to determine the bulk_import_column instead of checking for each batch record

Ah - didn't make the connection between this and line 100. -_- Maybe rename associations to something else - it's mirroring record.associations but isn't actually recording the same thing.

In that case there's no need to use Hash.new([]).

dorner · 2024-05-09T13:32:09Z

lib/deimos/active_record_consume/mass_updater.rb

@@ -84,15 +87,60 @@ def import_associations(record_list)
        end
      end

+      # rubocop:disable Metrics/AbcSize


Let's not disable this - if it's complaining, the function is too big. Let's break it up.

dorner

I think this works!

lionelpereira requested a review from dorner May 8, 2024 17:34

dorner requested changes May 8, 2024

View reviewed changes

lionelpereira added 3 commits May 9, 2024 08:29

CCOL-2440: Add support for joined record batch consumption

ce10ecd

Update specs & use duplicate key update instead of ignore

d9fbe48

CR fixes

c1cbd9f

lionelpereira force-pushed the CCOL-2440 branch from 195cfdd to c1cbd9f Compare May 9, 2024 12:31

lionelpereira requested a review from dorner May 9, 2024 12:33

dorner requested changes May 9, 2024

View reviewed changes

Break out save_associations_first method

6df054e

lionelpereira requested a review from dorner May 9, 2024 14:06

dorner approved these changes May 9, 2024

View reviewed changes

dorner merged commit 98e478f into master May 9, 2024
5 of 6 checks passed

dorner deleted the CCOL-2440 branch May 9, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCOL-2440: Add consumer configuration to save associated keys prior to saving primary record #217

CCOL-2440: Add consumer configuration to save associated keys prior to saving primary record #217

lionelpereira commented May 7, 2024 •

edited

Loading

dorner commented May 8, 2024

lionelpereira commented May 8, 2024

dorner May 8, 2024

dorner May 8, 2024

dorner May 8, 2024

dorner May 8, 2024

lionelpereira May 8, 2024

dorner May 8, 2024

dorner May 9, 2024

lionelpereira May 9, 2024 •

edited

Loading

dorner May 9, 2024

dorner May 9, 2024

dorner left a comment

CCOL-2440: Add consumer configuration to save associated keys prior to saving primary record #217

CCOL-2440: Add consumer configuration to save associated keys prior to saving primary record #217

Conversation

lionelpereira commented May 7, 2024 • edited Loading

Pull Request Template

Description

How Has This Been Tested?

Checklist:

dorner commented May 8, 2024

lionelpereira commented May 8, 2024

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

lionelpereira May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

lionelpereira commented May 7, 2024 •

edited

Loading

lionelpereira May 9, 2024 •

edited

Loading