-
-
Notifications
You must be signed in to change notification settings - Fork 218
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.
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
Process Repeaters, Part 1 #35033
base: master
Are you sure you want to change the base?
Process Repeaters, Part 1 #35033
Changes from 12 commits
50c848a
0db6a6c
e36296c
aeb10ba
01e4bc7
db2fec2
85b952e
c28c11b
d8d9642
48c3d7c
418ed3a
85bbfa3
d1119bb
03b26cf
de27ba0
4955ef4
59aae71
f40e6f4
b70fc52
7e65b3b
4c41896
30d4a6f
fc0f174
e32b465
e3bcd74
efc4dde
bd37a00
a448b9e
4321fb7
74137f9
968a922
4fd14a0
07320b9
b1eb171
2463348
8a7f343
0f72ba9
7f18e52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("repeaters", "0013_alter_repeatrecord_state_and_more"), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name="repeater", | ||
name="max_workers", | ||
field=models.IntegerField(default=0), | ||
), | ||
] |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar question about putting migrations in a separate PR. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("repeaters", "0014_repeater_max_workers"), | ||
] | ||
|
||
operations = [ | ||
migrations.AlterField( | ||
model_name="repeatrecord", | ||
name="state", | ||
field=models.PositiveSmallIntegerField( | ||
choices=[ | ||
(1, "Pending"), | ||
(2, "Failed"), | ||
(4, "Succeeded"), | ||
(8, "Cancelled"), | ||
(16, "Empty"), | ||
(32, "Invalid Payload"), | ||
], | ||
db_index=True, | ||
default=1, | ||
), | ||
), | ||
migrations.AddIndex( | ||
model_name="repeater", | ||
index=models.Index( | ||
condition=models.Q(("is_paused", False)), | ||
fields=["next_attempt_at"], | ||
name="next_attempt_at_not_paused_idx", | ||
), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,7 @@ | |
from http import HTTPStatus | ||
from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse | ||
|
||
from django.conf import settings | ||
from django.db import models, router | ||
from django.db.models.base import Deferred | ||
from django.dispatch import receiver | ||
|
@@ -230,10 +231,19 @@ def all_ready(self): | |
repeat_records_ready_to_send = models.Q( | ||
repeat_records__state__in=(State.Pending, State.Fail) | ||
) | ||
return (self.get_queryset() | ||
.filter(not_paused) | ||
.filter(next_attempt_not_in_the_future) | ||
.filter(repeat_records_ready_to_send)) | ||
return ( | ||
self.get_queryset() | ||
.filter(not_paused) | ||
.filter(next_attempt_not_in_the_future) | ||
.filter(repeat_records_ready_to_send) | ||
) | ||
|
||
def get_all_ready_ids_by_domain(self): | ||
results = defaultdict(list) | ||
query = self.all_ready().values_list('domain', 'id') | ||
for (domain, id_uuid) in query.all(): | ||
results[domain].append(id_uuid.hex) | ||
return results | ||
|
||
def get_queryset(self): | ||
repeater_obj = self.model() | ||
|
@@ -260,6 +270,7 @@ class Repeater(RepeaterSuperProxy): | |
is_paused = models.BooleanField(default=False) | ||
next_attempt_at = models.DateTimeField(null=True, blank=True) | ||
last_attempt_at = models.DateTimeField(null=True, blank=True) | ||
max_workers = models.IntegerField(default=0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of spinning this feature off in a separate PR (if we decided it's needed)? We have a lot of knobs to turn already, and I wonder if we'll need this one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We needed this one years ago. OpenMRS integrations involve Events and Observations, and it is really useful if we can send repeat records in the order in which their forms were submitted. More generally, setting And setting it to [all of the workers] is an insurance policy for this PR to ensure that we can still handle the highest volume for repeaters that need it. I'd sleep easier if this is included. |
||
options = JSONField(default=dict) | ||
connection_settings_id = models.IntegerField(db_index=True) | ||
is_deleted = models.BooleanField(default=False, db_index=True) | ||
|
@@ -271,6 +282,13 @@ class Repeater(RepeaterSuperProxy): | |
|
||
class Meta: | ||
db_table = 'repeaters_repeater' | ||
indexes = [ | ||
models.Index( | ||
fields=['next_attempt_at'], | ||
condition=models.Q(is_paused=False), | ||
name='next_attempt_at_not_paused_idx', | ||
), | ||
] | ||
|
||
payload_generator_classes = () | ||
|
||
|
@@ -350,9 +368,24 @@ def _repeater_type(cls): | |
|
||
@property | ||
def repeat_records_ready(self): | ||
return self.repeat_records.filter(state__in=(State.Pending, State.Fail)) | ||
""" | ||
A QuerySet of repeat records in the Pending or Fail state in the | ||
order in which they were registered | ||
""" | ||
return ( | ||
self.repeat_records | ||
.filter(state__in=(State.Pending, State.Fail)) | ||
.order_by('registered_at') | ||
) | ||
|
||
@property | ||
def num_workers(self): | ||
# If num_workers is 1, repeat records are sent in the order in | ||
# which they were registered. | ||
num_workers = self.max_workers or settings.DEFAULT_REPEATER_WORKERS | ||
return min(num_workers, settings.MAX_REPEATER_WORKERS) | ||
|
||
def set_next_attempt(self): | ||
def set_backoff(self): | ||
now = datetime.utcnow() | ||
interval = _get_retry_interval(self.last_attempt_at, now) | ||
self.last_attempt_at = now | ||
|
@@ -365,7 +398,7 @@ def set_next_attempt(self): | |
next_attempt_at=now + interval, | ||
) | ||
|
||
def reset_next_attempt(self): | ||
def reset_backoff(self): | ||
if self.last_attempt_at or self.next_attempt_at: | ||
self.last_attempt_at = None | ||
kaapstorm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.next_attempt_at = None | ||
|
@@ -977,11 +1010,17 @@ def get_domains_with_records(self): | |
class RepeatRecord(models.Model): | ||
domain = models.CharField(max_length=126) | ||
payload_id = models.CharField(max_length=255) | ||
repeater = models.ForeignKey(Repeater, | ||
on_delete=DB_CASCADE, | ||
db_column="repeater_id_", | ||
related_name='repeat_records') | ||
state = models.PositiveSmallIntegerField(choices=State.choices, default=State.Pending) | ||
repeater = models.ForeignKey( | ||
Repeater, | ||
on_delete=DB_CASCADE, | ||
db_column="repeater_id_", | ||
related_name='repeat_records', | ||
) | ||
state = models.PositiveSmallIntegerField( | ||
choices=State.choices, | ||
default=State.Pending, | ||
db_index=True, | ||
) | ||
registered_at = models.DateTimeField() | ||
next_check = models.DateTimeField(null=True, default=None) | ||
max_possible_tries = models.IntegerField(default=MAX_BACKOFF_ATTEMPTS) | ||
|
@@ -1161,7 +1200,9 @@ def fire(self, force_send=False, timing_context=None): | |
self.repeater.fire_for_record(self, timing_context=timing_context) | ||
except Exception as e: | ||
self.handle_payload_error(str(e), traceback_str=traceback.format_exc()) | ||
raise | ||
finally: | ||
return self.state | ||
kaapstorm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return None | ||
|
||
def attempt_forward_now(self, *, is_retry=False, fire_synchronously=False): | ||
from corehq.motech.repeaters.tasks import ( | ||
|
@@ -1171,6 +1212,19 @@ def attempt_forward_now(self, *, is_retry=False, fire_synchronously=False): | |
retry_process_datasource_repeat_record, | ||
) | ||
|
||
def is_new_synchronous_case_repeater_record(): | ||
""" | ||
Repeat record is a new record for a synchronous case repeater | ||
See corehq.motech.repeaters.signals.fire_synchronous_case_repeaters | ||
""" | ||
return fire_synchronously and self.state == State.Pending | ||
|
||
if ( | ||
toggles.PROCESS_REPEATERS.enabled(self.domain, toggles.NAMESPACE_DOMAIN) | ||
and not is_new_synchronous_case_repeater_record() | ||
): | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider the scenario where this toggle is enabled and then later disabled. With There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If by "gracefully" you mean "will all repeat records be processed when the toggle is disabled?", then yes. But if by "gracefully" you mean "will Datadog show the repeat records as not overdue?", then no. Datadog will show the repeat records as overdue. Alternatively, when we apply a back-off to a repeater, we could also update all its pending and failed repeat records too. I considered this, but it felt like a lot of churn on the repeat records table. I thought a better approach would be to use a new metric for Datadog to gauge when the repeat records queue is getting backed up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
if self.next_check is None or self.next_check > datetime.utcnow(): | ||
return | ||
|
||
|
@@ -1327,3 +1381,11 @@ def domain_can_forward(domain): | |
domain_has_privilege(domain, ZAPIER_INTEGRATION) | ||
or domain_has_privilege(domain, DATA_FORWARDING) | ||
) | ||
|
||
|
||
@quickcache(['domain'], timeout=60) | ||
kaapstorm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def domain_can_forward_now(domain): | ||
millerdev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return ( | ||
domain_can_forward(domain) | ||
and not toggles.PAUSE_DATA_FORWARDING.enabled(domain) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the complexity of other things going on in this PR, it would be nice to have this migration in a separate PR so we don't need to worry about concerns around reverting the migration if this PR is reverted.