Add rollback and retry logic #34887

hacodeorg · 2020-05-19T23:57:58Z

PLC-840 This PR does 3 main things:

Breaking the pipeline into 3 main parts: collecting & processing contacts, syncing new contacts to Pardot, and syncing updated contacts to Pardot. This allows each part to run independently. Failures in syncing new contacts Pardot do not affect syncing updated contacts to Pardot and vice versa.
Wraping all data modification queries in transactions so they can be rolled back properly when they fail.
Adding retrying with exponential backoff for downloading Pardot prospects and updating existing prospects.

Other smaller things (I must admit they could be in a separate PR):

Encapsulating logging responsibility into ContactRollupsV2 object.
Moving query building in ContactRollupsProcessed to its own function. It will be easier to write test for this query or test it on a mysql client.
Cleaning up LogCollector class.

Testing story

Unit tests
[Ongoing] Testing on an adhoc with connection to a production-clone database.

Reviewer Checklist:

Tests provide adequate coverage
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

hacodeorg · 2020-05-20T08:09:25Z

dashboard/app/models/contact_rollups_processed.rb

  # @param [Integer] batch_size number of records to save per INSERT statement.
  def self.import_from_raw_table(batch_size = DEFAULT_BATCH_SIZE)
+    # Process the aggregated data row by row and save the results to DB in batches.


There are no logic changes in this file. Just move the query builder into a separate method.

bencodeorg · 2020-05-20T19:48:16Z

dashboard/app/models/contact_rollups_final.rb

@@ -23,6 +23,6 @@ def self.insert_from_processed_table
      FROM contact_rollups_processed;
    SQL

-    ActiveRecord::Base.connection.exec_query(insert_sql)
+    transaction {ActiveRecord::Base.connection.exec_query(insert_sql)}


Is this just for consistency with other places where we're using transactions? Or is there actually a benefit of wrapping this in a transaction?

There is one benefit, ContactRollupsFinal will be empty if this fails, thus there is no risk of accidentally using incomplete data.

bencodeorg · 2020-05-20T20:02:43Z

dashboard/lib/contact_rollups_v2.rb

+
+    # These sync steps are independent, one could fail without affecting another.
+    # However, if the build step above fails, none of them should run.
+    sync_new_contacts_with_pardot


So we have no more sync_with_pardot flag, Pardot steps will happen by default. Is that a problem?

Right, I forgot about that! In bin/cron/build_contact_rollups_v2, I will call collect_and_process_contacts instead of build_and_sync.

bencodeorg · 2020-05-20T20:03:47Z

lib/cdo/log_collector.rb

@@ -38,9 +38,10 @@ def time(action_name = nil)
    )
    record_exception(e)
  end
+  alias_method :time_and_continue, :time


Could you explain this to me? 😄

It is mostly for backward compatibility; I create an alias for the method instead of renaming it.

Background:
In LogCollector, time method measures time to execute a block and also capture any StandardError exceptions if they arise. It doesn't re-raise exception to avoid disrupting the flow of its caller.

Meanwhile, time! method, alias: time_and_raise!, will bubble up any errors occurs.

bencodeorg · 2020-05-20T20:07:55Z

lib/cdo/contact_rollups/v2/pardot.rb

@@ -113,7 +115,7 @@ def self.build_prospect_query_url(id_greater_than, fields, limit, deleted)
  def batch_create_prospects(email, data, eager_submit = false)
    prospect = self.class.convert_to_pardot_prospect data.merge(email: email)
    @new_prospects << prospect
-
+    # Creating new prospects is not a retriable action, unlike updating existing prospects.


Is this better?

Suggested change

# Creating new prospects is not a retriable action, unlike updating existing prospects.

# Creating new prospects is not a retriable action because it could succeed

# on the Pardot side and we just didn't receive a response. If we try again,

# it would create duplicate prospects.

bencodeorg · 2020-05-20T20:30:03Z

lib/test/cdo/contact_rollups/test_pardot_helpers.rb

@@ -33,4 +33,22 @@ def test_raise_if_response_error_no_error

    assert_nil PardotHelpersTest.send(:raise_if_response_error, pardot_ok)
  end
+
+  def test_try_with_exponential_backoff_one_retry
+    tries = 0


If I understand correctly this is a way to track the number of tries that actually occurred in this test? Possible to name this something else (eg, tries_counter? Got confused looking back and forth to method which also has a counter called tries?

Also might just be more clear to someone with more experience with blocks/retry/variable scope/etc.

Good point, I will change the variable name.

bencodeorg · 2020-05-20T20:40:44Z

lib/test/cdo/contact_rollups/test_pardot_helpers.rb

+    tries = 0
+    max_tries = 2
+    assert_raises RuntimeError do
+      PardotHelpersTest.try_with_exponential_backoff(max_tries, [RuntimeError]) do


Sorry this is more of a question hour than a proper PR review :) Why do we use the test class here instead of just using the class itself like we do elsewhere?

No problem at all 😄 Since PardotHelpers is a module, we have to test it by mixing it into a test class. In this case, the module is mixed into PardotHelpersTest using extend. (extend vs. include)

bencodeorg

LGTM! Mostly just a bunch of questions for my learning, and would love to figure out how you're thinking about handling not accidentally syncing to Pardot before merging.

Encapsulate logs collecting and results reporting in ContactRollupsV2 class.

…l in transactions.

This reverts commit e913aaa.

This reverts commit a07ed91.

hacodeorg added the contact-rollups label May 19, 2020

hacodeorg force-pushed the ha/cr-retry branch 2 times, most recently from b5548c8 to 28e1331 Compare May 20, 2020 01:24

hacodeorg changed the title ~~Add retry/rollback logics~~ Add rollback and retry logic May 20, 2020

hacodeorg marked this pull request as ready for review May 20, 2020 08:07

hacodeorg requested review from bencodeorg and a team May 20, 2020 08:10

hacodeorg commented May 20, 2020

View reviewed changes

bencodeorg reviewed May 20, 2020

View reviewed changes

bencodeorg approved these changes May 20, 2020

View reviewed changes

hacodeorg added 15 commits May 26, 2020 10:52

Break the pipeline into 2 parts that can run/rerun independently.

4bc9c06

Encapsulate logs collecting and results reporting in ContactRollupsV2 class.

Add method aliases and minor fixes for LogCollector.

fd3e19c

Tolerate failures in some extraction steps.

5da2a78

Wrap extraction queries in transactions

df6a3d5

Break query creation into its own function.

2b89a67

Minor fixes for readability

4b06611

Wrap operations in contact_rollups_processed and contact_rollups_fina…

5493fde

…l in transactions.

Revert "Tolerate failures in some extraction steps."

2b78f1a

This reverts commit e913aaa.

Revert "Wrap extraction queries in transactions"

79e58d5

This reverts commit a07ed91.

Wrap operations in contact_rollups_pardot_memory in transactions

8e46dbf

Break sync_contacts_with_pardot into 2 parts that can run independently.

ee43868

Simplify ContactRollupsV2 main functions.

a1b7e8f

Add retry method with exponential backoff.

cb35b5d

Minor fixes

ac809e7

Fix PR comments.

f380d4d

hacodeorg force-pushed the ha/cr-retry branch from 868e9fc to f380d4d Compare May 26, 2020 17:58

Fix unit tests

e1bc8ce

hacodeorg merged commit 6c599dd into staging May 26, 2020

hacodeorg deleted the ha/cr-retry branch May 26, 2020 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rollback and retry logic #34887

Add rollback and retry logic #34887

hacodeorg commented May 19, 2020 •

edited

hacodeorg May 20, 2020

bencodeorg May 20, 2020

hacodeorg May 20, 2020

bencodeorg May 20, 2020 •

edited

hacodeorg May 20, 2020

bencodeorg May 20, 2020 •

edited

hacodeorg May 20, 2020

bencodeorg May 20, 2020

hacodeorg May 20, 2020

bencodeorg May 20, 2020

bencodeorg May 20, 2020 •

edited

hacodeorg May 20, 2020

bencodeorg May 20, 2020

hacodeorg May 20, 2020

bencodeorg left a comment

-    # Creating new prospects is not a retriable action, unlike updating existing prospects.
+    # Creating new prospects is not a retriable action because it could succeed
+    # on the Pardot side and we just didn't receive a response. If we try again,
+    # it would create duplicate prospects.

Add rollback and retry logic #34887

Add rollback and retry logic #34887

Conversation

hacodeorg commented May 19, 2020 • edited

Testing story

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencodeorg May 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencodeorg May 20, 2020 • edited

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

bencodeorg May 20, 2020 • edited

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

bencodeorg left a comment

Choose a reason for hiding this comment

hacodeorg commented May 19, 2020 •

edited

bencodeorg May 20, 2020 •

edited

bencodeorg May 20, 2020 •

edited

bencodeorg May 20, 2020 •

edited