Closed Bug 1497159 Opened 11 months ago Closed 11 months ago

Pass expected timestamps during synchronization for cache busting

Categories

(Firefox :: Remote Settings Client, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

Details

Attachments

(1 file)

Currently we rely on the CloudFront CDN invalidation to obtain the latest data.

With the introduction of push notifications to trigger synchronization, we could run into some race conditions where the different endpoints are not invalidated consistently (eg. Bug 1484162).

The most efficient way to guarantee that the client fetches the latest data is to use cache busting. 

For example, when the latest version is obtained via a push notification, the client would poll changes with something like `?_expected={version}`.
Then, using the list of changes obtained, it would synchronize every collection using the same principle (a query param with the expected version to be fetched).
Pass the expected timestamp along in querystring
> Should we land this as-is or should we try to get the fixes into the kinto libraries first? I don't think they'll be difficult changes..

The test don't pass yet. 

I think we should land the necessary changes to kinto.js and kinto-http.js first.
Assignee: nobody → mathieu
Comment on attachment 9015265 [details]
Bug 1497159 - Use Megaphone payload for cache busting r?glasserc

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1497159

User impact if declined: The Megaphone feature that we are trying to roll out will probably not work. Due to cache invalidation issues, when we push changes to the browser, the browser will re-fetch changes that it already has. Instead of pushing changes out immediately, we will have to fall back to the 24-hour poll that we normally do.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Covered by automated tests.

String changes made/needed:
Attachment #9015265 - Flags: approval-mozilla-beta?
Sorry if I jumped the gun here, I'm trying to make sure this can go in as soon as possible even though it hasn't landed in Nighty yet.
Pushed by mleplatre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e3812faf4bc
Use Megaphone payload for cache busting r=glasserc
> Another thing I just noticed is that this got changed in bug 1440284 so we should probably keep it.

Too late. Should we open another bug (+patch) just for that?
Some other modules still use that ``this.EXPORTED_SYMBOLS``
Then let's not worry about it. I'll try to get the fix landed in the upstream library.
(In reply to Ethan Glasser-Camp (:glasserc) from comment #5)
> Comment on attachment 9015265 [details]
> Bug 1497159 - Use Megaphone payload for cache busting r?glasserc
> 
> [Beta/Release Uplift Approval Request]
> 
> Feature/Bug causing the regression: Bug 1497159
> 
> User impact if declined: The Megaphone feature that we are trying to roll
> out will probably not work. Due to cache invalidation issues, when we push
> changes to the browser, the browser will re-fetch changes that it already
> has. Instead of pushing changes out immediately, we will have to fall back
> to the 24-hour poll that we normally do.
> 
> Is this code covered by automated tests?: Yes
> 
> Has the fix been verified in Nightly?: No
> 
> Needs manual test from QE?: No
> 
> If yes, steps to reproduce: 
> 
> List of other uplifts needed: None
> 
> Risk to taking this patch: Low
> 
> Why is the change risky/not risky? (and alternatives if risky): Covered by
> automated tests.
> 
> String changes made/needed:

Hi glasserc,

Am I correct in understanding that us folks over in Test Engineering don't need to do anything over here since the test will be added to Kinto itself and is not a deployment-driven test like the suite Test Engineering maintains?
Flags: needinfo?(eglassercamp)
This change was entirely client-side, so not added to Kinto itself but rather part of Firefox. However, I think the rest of your question is correct -- typically I think you haven't done QA on the Firefox part of the remote-settings project.
Flags: needinfo?(eglassercamp)
https://hg.mozilla.org/mozilla-central/rev/8e3812faf4bc
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9015265 [details]
Bug 1497159 - Use Megaphone payload for cache busting r?glasserc

Pascal and I both reviewed it and we don't believe this is a low risk change based on the size of the patch. Not uplifting this may mean running into a race condition but atm there are no real plans to use Globalbroadcast in 63. We prefer to let this fix ride the 64 train.
Attachment #9015265 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.