Create RepeaterStubs #29599

kaapstorm · 2021-04-23T09:59:30Z

Summary

Repeat Records Couch-to-SQL migration PR 4 of 6

CEP

I'm opening this as a draft PR to give context to the conversation around this migration. Following on for a conversation with Danny, this and the two PRs that build on it will need some changes, in order to roll out this migration with more caution.

The changes to these PRs that we discussed are:

Mirroring repeat record creation and updates across both Couch and SQL (following migration best practices) to allow us to switch from one backend to the other as easily as possible.
Repeat records and RepeaterStub to live in their own database.
Splitting out the change in behavior from the migration.

Currently this series of PRs introduces a change in behavior enabled by the switch to SQL; instead of iterating repeat records we can iterate repeaters, send payloads in the order they were created, and handle offline services more intelligently. But to reduce the risk inherent in behavior changes, we should deploy the behavior change after the switch to SQL, instead of with it.

Safety Assurance

Risk label is set correctly
All migrations are backwards compatible and won't block deploy
The set of people pinged as reviewers is appropriate for the level of risk of the change
If QA is part of the safety story, the "Awaiting QA" label is used
I have confidence that this PR will not introduce a regression for the reasons below

Automated test coverage

PR includes test coverage. If there are aspects that are not covered by tests, please point them out.

QA Plan

Left to the discretion of the SaaS team.

Safety story

This PR will be subject to change. It is not safe to merge.

Rollback instructions

This PR can be reverted after deploy with no further considerations

stickler-ci · 2021-04-23T09:59:42Z

corehq/motech/repeaters/tests/test_models.py

@@ -402,3 +400,122 @@ def test_yes(self):
        with make_repeat_record(self.repeater_stub, RECORD_PENDING_STATE):
            is_migrated = are_repeat_records_migrated(DOMAIN)
        self.assertTrue(is_migrated)
+
+
+class RepeaterStubOneToOneRepeaterTests(RepeaterFixtureMixin, TestCase):


F821 undefined name 'RepeaterFixtureMixin'

stickler-ci · 2021-04-23T09:59:42Z

corehq/motech/repeaters/tests/test_models.py

+        ).count(), 0)
+
+
+class TestMigrationCantDuplicate(RepeaterFixtureMixin, TestCase):


F821 undefined name 'RepeaterFixtureMixin'

stickler-ci · 2021-04-23T09:59:42Z

corehq/motech/repeaters/tests/test_models.py

+        ).count(), 1)
+
+
+class PauseResumeRetireRepeaterTests(RepeaterFixtureMixin, TestCase):


F821 undefined name 'RepeaterFixtureMixin'

mjriley

The changes look good, although I'm concerned with how the two systems handle downtime. I'd like to see some test changes, and, if possible, RepeaterStub to be called anything other than Stub

mjriley · 2021-04-23T19:07:45Z

corehq/motech/repeaters/migration_functions.py

+                # It's faster to violate uniqueness constraint and ask
+                # forgiveness than to use `.get()` or `.exists()` first.
+                pass


Wouldn't get_or_create be the canonical way to do this?

mjriley · 2021-04-23T19:08:27Z

corehq/motech/repeaters/migration_functions.py

+
+def create_repeaterstubs(apps, schema_editor):
+    for repeater in iter_repeaters():
+        with transaction.atomic():


Does this need to be a transaction? Unless something will be added later, this is only performing a single create, which is going to be auto-committed anyhow

mjriley · 2021-04-23T19:58:33Z

corehq/motech/repeaters/tests/test_models.py

+
+    def test_only_one_in_domain(self):
+        with transaction.atomic():
+            # (Run in its own transaction so as not to mess with tearDown)


What error was this giving you? Failing to create a duplicate object should not affect tearDown

mjriley · 2021-04-23T20:11:24Z

corehq/motech/repeaters/tests/test_models.py

+class RepeaterStubOneToOneRepeaterTests(RepeaterFixtureMixin, TestCase):
+
+    def test_one_in_domain(self):
+        self.assertEqual(RepeaterStub.objects.filter(
+            domain=DOMAIN,
+            repeater_id=self.repeater.get_id,


These tests are difficult to follow. They are deceptively short, because all of the work is done in the setup methods. Ideally, tests should follow the AAA pattern (Arrange, Act, Assert). I want to be able to see what is being constructed, what you are doing on those constructed objects, and what ultimately is the expected result we're asserting against. In this case, all of our arrange blocks are hidden behind multiple layers -- they don't exist in the test body itself, they don't exist in the class, so they're rather cryptic to find in RepeaterFixtureMixin. Does this behavior warrant 4 tests? It seems like we're just testing Django's models.UniqueConstraint.

You're right. Test mix-ins are awful, and I'm not sure how practical these tests are. I'll rework this.

mjriley · 2021-04-23T20:14:29Z

corehq/motech/repeaters/tests/test_models.py

+        self.repeater = FormRepeater(
+            domain=DOMAIN,
+            url='https://www.example.com/api/',
+        )


FormRepeater is still a couch model, right? Do our tests get stuck if this repeater already exists?

mjriley · 2021-04-23T20:57:10Z

corehq/motech/repeaters/dbaccessors.py

@@ -185,6 +185,17 @@ def _iter_repeat_records_by_repeater(domain, repeater_id, chunk_size,
            yield doc['id']


+def iter_repeaters():


Does iter convey any useful information? Judging by how it's used, get_all_repeaters() makes its usage in create_repeaterstubs a bit easier for me to understand. It was not obvious to me that it was fetching all repeaters

Developers have been frustrated in the past when they discover that get_foo() returns a generator, instead of a list or tuple that can be iterated more than once. So I use iter_foo() to make callers aware that they aren't getting a list.

Maybe a type hint would check both boxes?

def get_all_repeaters() -> Generator: ...

mjriley · 2021-04-23T21:00:08Z

corehq/motech/repeaters/migration_functions.py

+
+def create_repeaterstubs(apps, schema_editor):
+    for repeater in iter_repeaters():
+        with transaction.atomic():


Any thought given to doing this process via bulk_create? It should be substantially faster

mjriley · 2021-04-23T21:05:43Z

corehq/motech/repeaters/models.py

+        constraints = [
+            models.UniqueConstraint(fields=['repeater_id'],
+                                    name='one_to_one_repeater')
+        ]


why isn't this done using unique?

mjriley · 2021-04-23T21:10:20Z

corehq/motech/repeaters/models.py

+    def delete(self):
+        if self.repeater_stub.id:
+            self.repeater_stub.delete()
+        super().delete()


Since these are two separate databases, it seems possible that deleting the stub could succeed, while deleting from the SQL database could fail. Is that a situation that needs to be avoided? If so, it seems like it would be slightly safer to wrap this in a transaction so we can rollback the stub's deletion in the event that the couch deletion fails.

mjriley · 2021-04-23T21:14:01Z

corehq/motech/repeaters/models.py

+        # Deleting RepeaterStub will cascade-delete SQLRepeatRecords
+        # and SQLRepeatRecordAttempts too. (RequestLog will still keep a
+        # record of all send attempts.)
+        self.repeater_stub.delete()
+        # NOTE: Undeleting a Repeater needs to include creating a
+        #       RepeaterStub for it.


In this, pause, and resume, are we concerned about what happens if couch or SQL are not both simultaneously available? Again, using SQL transactions might be useful to mitigate that here. It might also be useful to have at least one test that illustrates how we handle when one of the systems is not available

kaapstorm · 2021-07-26T13:37:57Z

Thanks for the review @mjriley

I'll be spending some time changing the way this migration is rolled out, and I'll take the opportunity to implement the feedback you've given here and on the "Switch over" PR.

I am looking for suggestions for what to rename "RepeaterStub". Its purpose is to link a Repeater's repeat records, and allow them to be managed collectively (which we aren't doing with Couch repeat records). I don't like the current name either, but I couldn't think of a good name.

kaapstorm added 9 commits April 21, 2021 10:18

RepeaterStub UniqueConstraint

1f4ed4e

Creating a Repeater creates a RepeaterStub

7fca24f

⚖ Update tests

8c911c2

⚖ Test RepeaterStub UniqueConstraint

ee54af4

⚖ Test saving and deleting a Repeater

363f4c2

Migration to create RepeaterStubs

c5708f2

⚖ Test migration to create RepeaterStubs

aef4d1d

Pause, resume and retire RepeaterStubs

14b665e

⚖ Test pause, resume, retire

1fe3fbc

kaapstorm added the product/invisible Change has no end-user visible impact label Apr 23, 2021

kaapstorm requested review from dannyroberts, millerdev, orangejenny and gherceg April 23, 2021 09:59

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Apr 23, 2021

stickler-ci reviewed Apr 23, 2021

View reviewed changes

kaapstorm requested a review from mjriley April 23, 2021 10:01

This was referenced Apr 23, 2021

Management commands #29600

Draft

Switch over #29601

Draft

mjriley reviewed Apr 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create RepeaterStubs #29599

Create RepeaterStubs #29599

kaapstorm commented Apr 23, 2021 •

edited

Loading

stickler-ci Apr 23, 2021

stickler-ci Apr 23, 2021

stickler-ci Apr 23, 2021

mjriley left a comment

mjriley Apr 23, 2021

mjriley Apr 23, 2021

mjriley Apr 23, 2021

mjriley Apr 23, 2021

kaapstorm Jul 26, 2021

mjriley Apr 23, 2021

mjriley Apr 23, 2021

kaapstorm Jul 26, 2021

mjriley Apr 23, 2021

mjriley Apr 23, 2021

mjriley Apr 23, 2021

mjriley Apr 23, 2021

kaapstorm commented Jul 26, 2021

		).count(), 0)


		class TestMigrationCantDuplicate(RepeaterFixtureMixin, TestCase):

		).count(), 1)


		class PauseResumeRetireRepeaterTests(RepeaterFixtureMixin, TestCase):

		@@ -185,6 +185,17 @@ def _iter_repeat_records_by_repeater(domain, repeater_id, chunk_size,
		yield doc['id']


		def iter_repeaters():

Create RepeaterStubs #29599

Are you sure you want to change the base?

Create RepeaterStubs #29599

Conversation

kaapstorm commented Apr 23, 2021 • edited Loading

Summary

Repeat Records Couch-to-SQL migration PR 4 of 6

Safety Assurance

Automated test coverage

QA Plan

Safety story

Rollback instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjriley 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaapstorm commented Jul 26, 2021

kaapstorm commented Apr 23, 2021 •

edited

Loading