Couch to SQL Repeat Records PR 1: Write to SQL #33978

millerdev · 2024-01-15T21:52:33Z

This is the first PR in the Couch to SQL migration process of Repeat Records. Commits for PR 2 (and eventually PR 3) can be found on dm/sql-repeatrecord.

🐠 Review by commit.

https://dimagi-dev.atlassian.net/browse/SAAS-14299

Safety Assurance

Safety story

Automated tests have been added where necessary to cover new functionality and to verify that Couch Repeat Records are synced to SQL as expected. In general, TDD was used to develop this PR. Repeater functionality should be unchanged.

Automated test coverage

Yes.

QA Plan

https://dimagi-dev.atlassian.net/browse/QA-6025

Rollback instructions

A decision will need to be made about what to do with data in SQLRepeatRecord and SQLRepeatRecordAttempt tables If this PR is reverted. Otherwise, this PR can be reverted after deploy with no further considerations.

cached_property is less complicated and easier to patch.

Syncing submodels using the Couch document model is expensive. It would be wildly inefficient to rewrite all attempts every time a repeat record is saved, not to mention when a new attempt is being added. However, proper submodel syncing is needed when copying old documents from Couch to SQL. New attempts are only added in non-migration code in one place, RepeatRecord.add_attempt(), which will be addressed in another commit.

minhaminha · 2024-01-19T15:42:51Z

corehq/motech/repeaters/tests/test_dbaccessors.py

@@ -50,7 +67,6 @@ def setUpClass(cls):
            domain=cls.domain,
            succeeded=True,
            repeater_id=cls.repeater_id,
-            next_check=before,


Why was this line removed/how was it breaking the tests?

It is not valid to have a next_check date in SQL when state == Success.

commcare-hq/corehq/motech/repeaters/models.py

Lines 1366 to 1373 in f8abe92

models.CheckConstraint(

name="next_check_pending_or_null",

check=(

models.Q(next_check__isnull=True)

| models.Q(next_check__isnull=False, state=State.Pending)

| models.Q(next_check__isnull=False, state=State.Fail)

)

),

minhaminha · 2024-01-19T15:49:57Z

corehq/apps/data_interfaces/tests/test_utils.py

-                repeater=cls.repeater,
-                registered_at=now,
-            ))
+        cls.sql_records = list(SQLRepeatRecord.objects.filter(


nit: You can get rid of line 534 (cls.sql_records = [])

Thanks. This gets refactored in a later commit (it will be in the next PR).

AmitPhulera

Looking good overall. Just a minor comment on tests.

I am assuming this is already in process of QA and is deployed on staging?

AmitPhulera · 2024-01-21T10:30:59Z

corehq/motech/repeaters/models.py

+        self.succeeded = False
+        self.cancelled = False
+        try:
+            reason = self.failure_reason


Why will this raise AssertionError?

jsonobject's JsonProperty raises AssertionError if the record does not have a failure_reason. That's annoying. It should raise AttributeError, which would allow standard use of getattr.

AmitPhulera · 2024-01-21T10:59:52Z

corehq/motech/repeaters/models.py

+        # NOTE a 204 status here could mean either
+        # 1. the request was not sent, in which case response is
+        #    probably RepeaterResponse(204, "No Content")
+        # 2. the request was sent, and the remote end responded


Do you think it is required to specify this in user facing docs as well?

Possibly if we want to keep it this way. I think a better long-term fix would be to change the way we internally signal that the request was not sent so there is no ambiguity.

At some point (see docstring) we internally signaled that the request was not sent by passing True.

How about ...

if response is True or ( isinstance(response, RepeaterResponse) and response.status_code == 204 ): state = State.Empty else: state = State.Success

... and in the future we can come up with a better, and consistent, internal signal?

@kaapstorm can you point to the place(s) where response is True originates? I don't see anywhere that does that. Is it possible that they were all changed from return True to return RepeaterResponse(204, "No content")?

Edit: a test asserts that a (SQL) record created with response=True results in state == Success (not Empty). Should that test be changed?

commcare-hq/corehq/motech/repeaters/tests/test_models.py

Lines 304 to 305 in d624c4a

self.repeat_record.add_success_attempt(response=True)

self.assertEqual(self.repeat_record.state, RECORD_SUCCESS_STATE)

Is it possible that they were all changed from return True to return RepeaterResponse(204, "No content")?

I checked. They were all changed:

Repeater.send_request() -> requests.Response Dhis2Repeater.send_request() -> requests.Response | RepeaterResponse FHIRRepeater.send_request() -> requests.Response | RepeaterResponse ReferCaseRepeater.send_request() -> requests.Response | RepeaterResponse DataRegistryRepeater.send_request() -> requests.Response | RepeaterResponse OpenmrsRepeater.send_request() -> RepeaterResponse Dhis2EntityRepeater.send_request() -> RepeaterResponse

We should update that docstring, in a separate PR.

a test asserts that a (SQL) record created with response=True results in state == Success (not Empty). Should that test be changed?

I guess that test can be removed in the PR that updates the docstring.

corehq/motech/repeaters/tests/test_couchsqlmigration.py

millerdev · 2024-01-23T18:22:27Z

I am assuming this is already in process of QA and is deployed on staging?

Correct.

dannyroberts

Looks great. For parts I couldn't follow super well, I looked for tests—and the way you paired small test changes with each commit made that easy!

dannyroberts · 2024-01-16T20:50:06Z

corehq/motech/repeaters/models.py

@@ -1049,8 +1049,7 @@ def wrap(cls, data):
            )]
        return self

-    @property
-    @memoized
+    @cached_property


Just want to note that cached_property and memoized + property have somewhat different semantics; if you try to directly write to a memoized property it raises an error, whereas if you try to directly write to a cached_property it will let you override the value defined by the decorated method. https://docs.python.org/3/library/functools.html#functools.cached_property

Not sure if that is disqualifying, but wanted to make sure you'd considered that.

Yes, not disqualifying.

dannyroberts · 2024-01-16T20:54:26Z

corehq/motech/repeaters/management/commands/populate_repeatrecords.py

+
+        May include repeaters that have been created since the migration
+        started, whose records are already migrated. Also ignore records
+        associated with deleted repeaters.


This sounds probably fine, but to confirm, what is the behavior now of records for deleted repeaters? Are they hidden from the UI and any other places the user might be able to see that they're still in the system? Or are the effectively abandoned and never shown to the user?

This "deleted" is referring to hard-deleted repeaters. Such repeat records cannot exist in SQL because of the foreign key relationship between (SQL)Repeater and SQLRepeatRecord. In Couch they could exist, but would be orphaned.

To answer your question more directly, it is possible to find them in the repeat record report, for example by searching by payload ID, but attempting to view responses or the payload result in an error:

Repeater with id ... could not be found

Clicking the button to Resend Payload or Requeue Payload appears to succeed, but when the record is processed it will not be forwarded because the repeater does not exist.

It seems like a bug that repeat records for deleted repeaters can be viewed at all since the information about where they were sent was deleted with the repeater.

dannyroberts · 2024-01-16T20:58:34Z

corehq/motech/repeaters/management/commands/populate_repeatrecords.py

+        def iter_domain_docs(domain):
+            return iter_docs(chunk_size, startkey=[domain], endkey=[domain, {}])
+        for domain in domains:
+            yield from iter_domain_docs(domain)


Where are these two new methods called? Couldn't find it in the PR diff

They're called by the Couch to SQL migration framework code when migrating individual domains.

commcare-hq/corehq/apps/cleanup/management/commands/populate_sql_model_from_couch_model.py

Lines 368 to 370 in bbe7d35

doc_count = self._get_couch_doc_count_for_domains(domains)

sql_doc_count = self._get_sql_doc_count_for_domains(domains)

docs = self._iter_couch_docs_for_domains(domains, chunk_size)

kaapstorm · 2024-01-25T12:19:04Z

corehq/motech/repeaters/models.py

+        # NOTE a 204 status here could mean either
+        # 1. the request was not sent, in which case response is
+        #    probably RepeaterResponse(204, "No Content")
+        # 2. the request was sent, and the remote end responded


At some point (see docstring) we internally signaled that the request was not sent by passing True.

How about ...

if response is True or ( isinstance(response, RepeaterResponse) and response.status_code == 204 ): state = State.Empty else: state = State.Success

... and in the future we can come up with a better, and consistent, internal signal?

kaapstorm · 2024-01-25T17:25:57Z

corehq/motech/repeaters/models.py

+            return super().save(*args, **kw)
+
+    def _migration_sync_submodels_to_sql(self, sql_object):
+        if self._should_sync_attempts:


I didn't follow the paths of what calls __migration_sync_to_sql(), but I'm guessing that you are sure that this cannot be reached outside the enable_attempts_sync_to_sql() context manager?

Yes, this is only called within the context of .save(), which uses the context manager. Having the attribute not set outside the context manager will alert us (hopefully in tests, if not then in Sentry) if any code path arises that violates that precondition.

Couch can have null attempt messages, SQL cannot.

next_check is only valid if the record is in Pending or Fail state.

millerdev · 2024-02-21T22:35:42Z

Latest commits address data states discovered while running populate_repeatrecords on staging.

corehq/motech/repeaters/tests/test_couchsqlmigration.py

AmitPhulera · 2024-02-22T09:55:32Z

corehq/motech/repeaters/tests/test_couchsqlmigration.py

+        self.assertEqual(len(obj.attempts), len(doc.attempts))
+        self.assertTrue(obj.attempts)
+
+    def test_migrate_record_with_unynced_sql_attempts2(self):


Suggested change

def test_migrate_record_with_unynced_sql_attempts2(self):

def test_migrate_record_with_unsynced_sql_attempts2(self):

What is the difference between this test and the test above it? What extra behaviour is it testing?

See 9e7d4d3, in which I renamed the test and added a comment.

millerdev added 15 commits January 15, 2024 08:18

Add Couch to SQL mixins to RepeatRecord classes

496f49f

Fix dbaccessors tests

7b57b51

Fix repeaters model tests

28dcf38

Adapt tests to repeat record migration

cee8fdc

Convert memoized property to cached property

b950402

cached_property is less complicated and easier to patch.

Resolve type checker syntax warning

d58762a

Set correct state on empty success response

dcb54ff

Fix requeue record empty state

481db11

Implement populate_repeatrecords migration

280da7d

Improve support for bulk submodel migration

92860cd

Fix command to migrate a subset of domains

bd7d8dc

Sync Repeat Record Attempts between Couch and SQL

ab5cb60

Sync to SQL on RepeatRecord.add_attempt(...)

1435b54

delete_duplicate_cancelled_records in SQL too

f8abe92

millerdev added awaiting QA QA in progress. Do not merge product/invisible Change has no end-user visible impact labels Jan 15, 2024

millerdev requested review from dannyroberts, AmitPhulera and gherceg January 15, 2024 21:52

millerdev requested a review from kaapstorm as a code owner January 15, 2024 21:52

millerdev changed the title ~~Couch to SQL Repeat Records: PR 1~~ Couch to SQL Repeat Records PR 1: Write to SQL Jan 15, 2024

minhaminha reviewed Jan 19, 2024

View reviewed changes

AmitPhulera approved these changes Jan 23, 2024

View reviewed changes

dannyroberts approved these changes Jan 23, 2024

View reviewed changes

Assert SQL database state in addition to logs

d624c4a

dannyroberts approved these changes Jan 25, 2024

View reviewed changes

Silence chatty migration when it has nothing to say

bebadbe

Fix populate_repeatrecords --override-is-migration-completed

e940fa8

kaapstorm approved these changes Jan 25, 2024

View reviewed changes

millerdev added 10 commits February 14, 2024 10:56

Improve instructions for fixing diffs

6a97db7

Convert null attempt message to empty string

2549147

Couch can have null attempt messages, SQL cannot.

Sync missing attempts

9751a70

Show fixup-diffs message if there were diffs

3814173

Handle null RepeatRecord.registered_on in Couch

099f33e

Do not diff failure_reason if Couch value is empty

ecb2bc9

Handle very old repeat records with no attempts

e50cf7a

Prefetch attempts for migration verification

c50d3bb

Ignore invalid next_check in Couch

3d1d09e

next_check is only valid if the record is in Pending or Fail state.

Handle old RepeatRecord docs with missing fields

9817670

millerdev requested review from kaapstorm, AmitPhulera and dannyroberts February 21, 2024 22:33

AmitPhulera approved these changes Feb 22, 2024

View reviewed changes

millerdev added 4 commits February 22, 2024 06:57

Fix test names and add explanatory comment

9e7d4d3

Preserve ignored count on fixup diffs

73d9af1

Do not block on negative count of items to migrate

159423e

Do not set ignored count on unfinished migration

b1ade14

AmitPhulera approved these changes Feb 22, 2024

View reviewed changes

millerdev added QA Passed and removed awaiting QA QA in progress. Do not merge labels Feb 22, 2024

millerdev merged commit edee01c into master Feb 22, 2024
13 checks passed

millerdev deleted the dm/sql-repeatrecord-pr1 branch February 22, 2024 21:19

millerdev mentioned this pull request Mar 6, 2024

Couch to SQL Repeat Records PR 2: Read from SQL #34236

Merged

2 tasks

millerdev mentioned this pull request Apr 2, 2024

Couch to SQL Repeat Records PR 3: Cleanup #34371

Merged

2 tasks

millerdev mentioned this pull request Jun 11, 2024

Couch to SQL Repeat Records: post-PR-3 migrations #34751

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Couch to SQL Repeat Records PR 1: Write to SQL #33978

Couch to SQL Repeat Records PR 1: Write to SQL #33978

millerdev commented Jan 15, 2024 •

edited

Loading

minhaminha Jan 19, 2024

millerdev Jan 22, 2024

minhaminha Jan 19, 2024

millerdev Jan 22, 2024

AmitPhulera left a comment

AmitPhulera Jan 21, 2024

millerdev Jan 23, 2024

AmitPhulera Jan 21, 2024

millerdev Jan 23, 2024

kaapstorm Jan 25, 2024

millerdev Jan 25, 2024 •

edited

Loading

kaapstorm Jan 26, 2024

millerdev commented Jan 23, 2024

dannyroberts left a comment

dannyroberts Jan 16, 2024

millerdev Jan 24, 2024

dannyroberts Jan 16, 2024

millerdev Jan 24, 2024

dannyroberts Jan 16, 2024

millerdev Jan 24, 2024

kaapstorm Jan 25, 2024

kaapstorm Jan 25, 2024

millerdev Jan 25, 2024

millerdev commented Feb 21, 2024

AmitPhulera Feb 22, 2024

millerdev Feb 22, 2024

	models.CheckConstraint(
	name="next_check_pending_or_null",
	check=(
	models.Q(next_check__isnull=True)
	\| models.Q(next_check__isnull=False, state=State.Pending)
	\| models.Q(next_check__isnull=False, state=State.Fail)
	)
	),

	self.repeat_record.add_success_attempt(response=True)
	self.assertEqual(self.repeat_record.state, RECORD_SUCCESS_STATE)

	doc_count = self._get_couch_doc_count_for_domains(domains)
	sql_doc_count = self._get_sql_doc_count_for_domains(domains)
	docs = self._iter_couch_docs_for_domains(domains, chunk_size)

	def test_migrate_record_with_unynced_sql_attempts2(self):
	def test_migrate_record_with_unsynced_sql_attempts2(self):

Couch to SQL Repeat Records PR 1: Write to SQL #33978

Couch to SQL Repeat Records PR 1: Write to SQL #33978

Conversation

millerdev commented Jan 15, 2024 • edited Loading

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

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

AmitPhulera left a 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

millerdev Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millerdev commented Jan 23, 2024

dannyroberts left a 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

Choose a reason for hiding this comment

millerdev commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millerdev commented Jan 15, 2024 •

edited

Loading

millerdev Jan 25, 2024 •

edited

Loading