ref(crons): Guard clock ticks against desynced-partitions #54489
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Backend
evanpurkhiser
commented
Aug 9, 2023
| @@ -180,19 +180,33 @@ def _try_handle_high_volume_task_trigger(ts: datetime): | |||
| reference_datetime = ts.replace(second=0, microsecond=0) | |||
| reference_ts = int(reference_datetime.timestamp()) | |||
|
|
|||
| existing_last_ts = redis_client.get(HIGH_VOLUME_LAST_TRIGGER_TS_KEY) | |||
There was a problem hiding this comment.
I don't particularly LOVE this variable name, any thoughts?
There was a problem hiding this comment.
precheck_last_ts? Idk if that's much better
wedamija
approved these changes
Aug 9, 2023
| @@ -180,19 +180,33 @@ def _try_handle_high_volume_task_trigger(ts: datetime): | |||
| reference_datetime = ts.replace(second=0, microsecond=0) | |||
| reference_ts = int(reference_datetime.timestamp()) | |||
|
|
|||
| existing_last_ts = redis_client.get(HIGH_VOLUME_LAST_TRIGGER_TS_KEY) | |||
There was a problem hiding this comment.
precheck_last_ts? Idk if that's much better
This fixes an issue with the original implementation of GH-54204 when processing messages in a non-monotonic order. Typically kafka messages will be in order like such 12:59:58 12:59:59 01:00:00 01:00:01 01:00:01 01:00:02 However, because of how messages are shared into the kafka partitions we may end up with a secnario that looks like this partitions #1 #2 #3 12:59:58 01:00:00 01:00:01 12:59:59 01:00:01 01:00:02 With one consumer reading from each partition sequentially we would read these out as 12:59:58 01:00:00 01:00:01 12:59:59 <-- problematic skip backwards in time 01:00:01 01:00:02 Prior to this change, when we would process the task_trigger clock tick for the timestamp `12:59:59` after `01:00:01` our `GETSET` would update the key with an OLDER timestamps. When the next tick happens at `01:00:01` we would now tick for the `01:00:00` minute boundary again incorrectly. This change corrects this by first looking at the existing last timestamp value stored in redis, if that value is smaller than the reference timestamp we're about to tick for, do nothing, do not store the older reference timestamp.
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-crons-guard-clock-ticks-against-desynced-partitions
branch
from
0601c4e to
bd9187d
Compare
evanpurkhiser
deleted the
evanpurkhiser/ref-crons-guard-clock-ticks-against-desynced-partitions
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue with the original implementation of GH-54204 when processing messages in a non-monotonic order.
Typically kafka messages will be in order like such
However, because of how messages are shared into the kafka partitions we may end up with a secnario that looks like this
With one consumer reading from each partition sequentially we would read these out as
Prior to this change, when we would process the task_trigger clock tick for the timestamp
12:59:59after01:00:01ourGETSETwould update the key with an OLDER timestamps.When the next tick happens at
01:00:01we would now tick for the01:00:00minute boundary again incorrectly.This change corrects this by first looking at the existing last timestamp value stored in redis, if that value is smaller than the reference timestamp we're about to tick for, do nothing, do not store the older reference timestamp.