-
Notifications
You must be signed in to change notification settings - Fork 12
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
Clarify usage of _expected
querystring parameter
#600
Conversation
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.
This is seems much clearer to me, but maybe it could be improved more by making it a bit more explicit (assuming that I'm understanding it correctly). WDYT?
Approving in any case, because it's an improvement either way and I'm not sure that my suggestion is an improvement.
I just read your comments in bugzilla and realize my understanding is totally wrong, since I wasn't really thinking about the push notification scenario. Let me update my comments. |
@@ -87,7 +87,7 @@ Returns the following response for the collection ``{cid}`` in the bucket ``{bid | |||
|
|||
.. note:: | |||
|
|||
The ``_expected={}`` querystring parameter is mandatory but can be set to ``0`` if unknown. See section below about cache busting. | |||
The ``_expected={}`` querystring parameter is mandatory. Either you pass the current collection timestamp value obtained when polling for changes in order to bust the CDN cache, or you use a hard-coded value (eg. ``0``) and rely on the cache TTL. See section below about cache busting. |
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 quite sure what "current collection timestamp" means. Is it the timestamp of the last fetch? The timestamp returned in the data for the last fetch? The current time? Whichever one it is, please spell out how it would work to bust the cache.
Can you lay out a scenario when you would want to bust the cache while polling? In general, it seems like I wouldn't want to do that.
Also, how can we use the _since
param in a cache-friendly manner? If all clients did something naive like use the timestamp they last polled, then it seems like each request would be a cache-miss.
@@ -108,7 +108,7 @@ Returns the list of collections and their current timestamp. | |||
|
|||
.. note:: | |||
|
|||
The ``_expected={}`` querystring parameter is mandatory but can be set to ``0`` if unknown. See next section about cache busting. | |||
The ``_expected={}`` querystring parameter is mandatory. Either you receive a Push notification from the server, and pass the timestamp value in order to bust the CDN cache, or you use a hard-coded value (eg. ``0``) and rely on the cache TTL. See section below about cache busting. |
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.
This one makes total sense to me. Is _expected
intended more for responding to push notifications than for polling?
Thanks for your feedback. I will rework and improve it. Some clarifications (will put them in the doc nicely later):
{
"metadata": {},
"timestamp": 1713532462683,
"changes": [
{
"id": "19e79f22-62cf-92e1-c12c-a3b4b9cf51be",
"last_modified": 1603126502200,
"bucket": "blocklists",
"collection": "plugins",
"host": "firefox.settings.services.mozilla.com"
},
{
"id": "b7f595f9-5fc5-d863-b5dd-e5425dcf427a",
"last_modified": 1604940558744,
"bucket": "blocklists",
"collection": "addons",
"host": "firefox.settings.services.mozilla.com"
},
...
We would have to agree on a definition of busting the cache. We have to make the distinction between always bust the cache (ie. use a random value in queryparam), and the first requests of each CDN node busting the cache. When polling with push notification, we want the first requests to bust the polling endpoint with the latest value. When polling without push notification, what I would recommend is to rely on the cache TTL for the polling endpoint (
For each collection, the amount of possible values for the timestamps is finite. Indeed, each timestamp value corresponds to datetime of the data publication (reviewer approving changes on the server). Hope it's clearer with these explanations 😅 but don't hesitate to insist :) I will try to apply these clarifications to the doc. |
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.
Somehow lint is unhappy, but these changes seem to add good clarity
That makes a lot of sense to me. I'd love to see you list out some of those example use cases in the docs, but that doesn't need to happen in this PR. |
@bendk do you find it clearer like this? Don't hesitate to ask more questions :) Thanks!