Switch over #29601
Switch over #29601
Conversation
kaapstorm
added
product/invisible
kaapstorm
requested review from
dannyroberts,
millerdev,
mjriley,
orangejenny and
gherceg
dimagimon
removed
the
Risk: High
| get_repeaters_by_domain, | ||
| iter_repeat_records_by_repeater, | ||
| ) | ||
| from ...models import Repeater, RepeaterStub |
There was a problem hiding this comment.
F401 '...models.RepeaterStub' imported but unused
|
|
||
| # Functions for evaluating scale | ||
| def get_repeat_record_domain_total(): | ||
| from corehq.motech.repeaters.dbaccessors import get_domains_that_have_repeat_records |
There was a problem hiding this comment.
Do we need this import? get_domains_that_have_repeat_records is in this same module
|
|
||
|
|
||
| def get_couch_repeater_total(): | ||
| from corehq.motech.repeaters.dbaccessors import get_domains_that_have_repeat_records |
There was a problem hiding this comment.
same comment as previous -- this import should be unnecessary
|
|
||
|
|
||
| def get_couch_repeat_record_total(): | ||
| from corehq.motech.repeaters.dbaccessors import get_domains_that_have_repeat_records |
There was a problem hiding this comment.
Same comment as previous -- this import should be unnecessary
|
|
||
| def get_couch_repeater_total(): | ||
| from corehq.motech.repeaters.dbaccessors import get_domains_that_have_repeat_records | ||
| from corehq.motech.repeaters.models import Repeater |
There was a problem hiding this comment.
Assuming this isn't a circular import, can this be moved to to the top of the file?
| repeater.get_id): | ||
| if couch_record.migrated: | ||
| continue | ||
| migrate_repeat_record.delay(repeater.repeater_stub, couch_record) |
There was a problem hiding this comment.
Is delay being used here as a form of multi-threading?
| @@ -0,0 +1,42 @@ | |||
| from inspect import cleandoc | |||
There was a problem hiding this comment.
This command (migrating couch repeater records to SQL) will have no use once migration is complete, right? Would a one-off script make more sense for this? I'd rather not create another command users need to ignore in ./manage.py (and I'd imagine it is a slight performance hit every time ./manage.py is executed)
There was a problem hiding this comment.
We usually end up checking scripts like this in if only because they also need to be run by third parties that host HQ. We can delete them after we're reasonably confident everyone should have run them.
This comment reminded me, @kaapstorm how will the migration experience be for third parties? Will this command be integrated into a django migration like in the auto-managed pattern? Apologies if you've already discussed this - I'm quite late to the party, haven't been paying close attention to this migration recently.
There was a problem hiding this comment.
(@kaapstorm ignore this question, I'm going to go read the doc about the repeaters migration plan)
| # Payload is already waiting to be sent | ||
| return | ||
|
|
||
| self.repeater_stub.repeat_records.create( |
There was a problem hiding this comment.
I think the model already exists for this specific PR, but if it's within scope, I'll advocate for renaming RepeaterStub, as Stub already has a well-defined meaning within testing.
| except: # noqa: E722 | ||
| logging.exception('Failed to migrate repeat record: ' | ||
| f'{couch_record.record_id}') | ||
| sql_record.delete() |
There was a problem hiding this comment.
What happens if this delete also fails? Do we have any logging in that circumstance?
Summary
Repeat Records Couch-to-SQL migration PR 6 of 6
See PR #29599 for more context.
The work I intended for this branch is not complete. You will notice "--wip--" commits at the end. I left myself a note to write the following tests to check that we don't create duplicate repeat records waiting to be sent:
Safety Assurance
Automated test coverage
Test coverage for this PR is not complete.
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
False^^^. PR #29600 includes documentation and a management command to be used if this PR needs to be reverted. But those should be moot, because the way this Couch-to-SQL migration is to be deployed will change.