-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Only send status checks when payload has changed #74
Conversation
Codecov Report
@@ Coverage Diff @@
## main #74 +/- ##
=======================================
Coverage 98.59% 98.59%
=======================================
Files 359 359
Lines 26284 26322 +38
=======================================
+ Hits 25914 25952 +38
Misses 370 370
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1f0c7b9
to
9c900df
Compare
9c900df
to
9cf9264
Compare
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.
looks good, left a comment about the cache key tho
) | ||
|
||
last_payload = cache.get_backend().get(cache_key) | ||
if last_payload is NO_VALUE or last_payload != payload: |
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.
if the payload is JSON serializable (not sure if it is), can we use the payload as the key?
It would save us the last_payload != payload
check... if the payload is there (as a key) we know we sent that before (in the last TTL seconds)
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.
I'm not sure that would work as expected. Imagine the following sequence of events...
- notify with payload X -> hashes to ABC (not in cache so we send notification)
- notify with payload X -> hashes to ABC (already cached, skip notification)
- notify with payload Y -> hashes to DEF (not in cache so we send notification)
- notify with payload X -> hashes to ABC (already in cache, skip notification)
We need to make sure that the last item there actually DOES send a notification.
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.
ah I see... Then indeed my understanding was incorrect.
I thought that case 4 there would indicate a task that got delayed for whatever reason (had to retry for example) and was "stale" data (because something more recent, in this case payload Y) was already delivered.
But indeed I see that case 4 would skip notification and it's not what we want.
Thanks for taking the time to explain.
Resolves codecov/engineering-team#350
Only send status notifications when the payload has changed from the last time we sent the same notification.