Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SQLRepeatRecord #28952

Merged
merged 15 commits into from
Jan 8, 2021
Merged

SQLRepeatRecord #28952

merged 15 commits into from
Jan 8, 2021

Conversation

kaapstorm
Copy link
Contributor

Summary

These changes are guided by [CEP] Migrate RepeaterRecord to SQL.

This PR establishes the groundwork for using SQL for data forwarding. It only adds models; it does not change any behavior.

(I am creating this PR as a draft because its base branch is PR #28948. When that PR is merged, I'll switch this to a real PR.)

cc @calellowitz @orangejenny @millerdev @snopoke

Safety Assurance

  • Risk label is set correctly
  • 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 am certain that this PR will not introduce a regression for the reasons below

Automated test coverage

Tests included in PR

QA Plan

Not yet applicable

Safety story

This PR does not change any behavior.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

@kaapstorm kaapstorm added the product/invisible Change has no end-user visible impact label Jan 4, 2021
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Jan 4, 2021
millerdev
millerdev reviewed Jan 5, 2021
View reviewed changes
Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. ❤️ 1970-01-06 comment

corehq/motech/repeaters/migrations/0002_sqlrepeatrecord.py Show resolved Hide resolved
@orangejenny
Copy link
Contributor

So exciting to see this happening 🥇

I didn't read the code closely but had a comment on naming: are you planning on keeping the SQL prefixes for class names? I've gotten critical feedback on that when doing migrations (and I've come around to that point of view, now I see the SQL as cruft). It's pretty painless to make the prefix temporary if you set db_table to match the un-prefixed name, then later you can remove the prefix, remove the old couch class, and remove the db_table in a cleanup PR.

@kaapstorm
Copy link
Contributor Author

are you planning on keeping the SQL prefixes for class names? I've gotten critical feedback on that when doing migrations

Good call!

Do you think RepeaterStub is a good name for SQLRepeaterStub? I'm not delighted by either, but I couldn't think of anything better.

@kaapstorm kaapstorm mentioned this pull request Jan 6, 2021
Merged
5 tasks
@orangejenny
Copy link
Contributor

I think RepeaterStub is okay. The only other thing that comes to mind is RepeaterJoiner but I don't think that's obviously better.

@kaapstorm
Copy link
Contributor Author

Urgh. I renamed it to RepeaterLink. I'm not sure it's an improvement, but I am sure I'm overthinking this. 😂

@orangejenny
Copy link
Contributor

orangejenny commented Jan 6, 2021 •
edited
Loading

Let me join you in overthinking...I don't really love RepeaterLink because "link" already has a meaning in web applications. I'd like to rename linked apps/domains for this same reason, we have some fairly confusing UI code where we're making <a> links that relate to linked domains. Not sure if that will come up with repeaters, but don't repeaters have URL attributes (which, granted, we probably call urls and not links)?

@kaapstorm
Copy link
Contributor Author

That is a good point. OK, I'll go with RepeaterStub.

@kaapstorm
Copy link
Contributor Author

(I kinda prefer RepeaterJoiner to RepeaterStub, but I just couldn't do RepeaterJoinerManager. I even (briefly) considered Peter (very, very briefly).)

proteusvacuum
proteusvacuum approved these changes Jan 7, 2021
View reviewed changes
Copy link
Contributor

@proteusvacuum proteusvacuum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

corehq/motech/repeaters/models.py Outdated
"""
return (
self.get_queryset()
# Is not paused
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this comment is redundant with the code. I think you wanted to make this code more readable, you could add in some named variables instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo! I like that idea! 2d8607d

@kaapstorm kaapstorm marked this pull request as ready for review January 7, 2021 19:21
Base automatically changed from nh/rep/refactor to master January 7, 2021 22:43
@kaapstorm
Update index hash
0784da8
@kaapstorm
Include in domain dump
a45e68e 
Should we really be dumping RepeatRecord and SQLRepeatRecord?
@kaapstorm
Domain deletion
85f8d7f
kaapstorm
kaapstorm commented Jan 8, 2021
View reviewed changes
corehq/apps/dump_reload/sql/dump.py
Comment on lines +144 to +147
# NH (2021-01-08): Including SQLRepeatRecord because we dump (Couch)
# RepeatRecord, but this does not seem like a good idea.
FilteredModelIteratorBuilder('repeaters.SQLRepeatRecord', SimpleFilter('domain')),
FilteredModelIteratorBuilder('repeaters.SQLRepeatRecordAttempt', SimpleFilter('repeat_record__domain')),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 99% of domains, including repeat records with dumped domain data is insignificant, but for a handful it could be gigantic, with very little benefit. I suspect that in the future, (SQL) repeat records will be deleted automatically after six to twelve months anyway.

I'd love to hear opinions on this, especially if you have dealt with domain migrations. (@esoergel @snopoke ?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can certainly exclude all SQLRepeatRecordAttempt objects. Is it possible to only include SQLRepeatRecord objects that have not been successful yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Yeah, we can filter by state.

@kaapstorm kaapstorm merged commit 7e5a03c into master Jan 8, 2021
@kaapstorm kaapstorm deleted the nh/rep/sqlrepeatrecord branch January 8, 2021 07:23
@kaapstorm kaapstorm mentioned this pull request Jan 11, 2021
Merged
5 tasks
@kaapstorm
Copy link
Contributor Author

ping @dannyroberts for context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orangejenny orangejenny orangejenny left review comments

@millerdev millerdev millerdev left review comments

@snopoke snopoke snopoke left review comments

@proteusvacuum proteusvacuum proteusvacuum approved these changes

@esoergel esoergel Awaiting requested review from esoergel

Assignees
No one assigned
Labels
product/invisible Change has no end-user visible impact reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kaapstorm @orangejenny @proteusvacuum @snopoke @millerdev @dimagimon