Introduce AMP Live List Cache Busting #25295
Conversation
…ier to each update of the live list
| // AMP Caches do not always evict entries from their caches. | ||
| // This experiment adds a random identifier to reduce cache hits for enrolled documents. | ||
| 'amp_random': String( | ||
| Math.floor(Math.random() * AMP_LIVE_LIST_MAX_RANDOM_NUMBER) |
There was a problem hiding this comment.
Did we decide to not go with the Date header approach?
There was a problem hiding this comment.
The Date header approach needs additional testing on older browsers where our polyfill is providing a stubbed Response object.
Given the timing for the fix, and the remedy on the Google AMP Cache – I instead opted for the more narrow fix for enrolled documents.
| this.manager_.register(this.liveListId_, this); | ||
| }); | ||
| return this.isExperimentEnabled_().then(enrolled => { | ||
| LiveListManager.forDoc(this.element, enrolled).then(manager => { |
There was a problem hiding this comment.
Will enrolled propagate to the LiveListManager constructor?
amphtml/extensions/amp-live-list/0.1/live-list-manager.js
Lines 123 to 128 in f49ba4e
Maybe we should do this check in LiveListManager on the first register() call instead. You can get an element from liveList.element in that function.
There was a problem hiding this comment.
Can refactor this way, will be a little bit.
There was a problem hiding this comment.
Moved over, mind taking another look?
jridgewell
changed the title
kristoferbaxter
changed the title
| 'amp_latest_update_time', | ||
| String(this.latestUpdateTime_) | ||
| ); | ||
| const parameters = this.enrolledInAppendRandomExperiment_ |
There was a problem hiding this comment.
I think there's a race condition here but the poll interval probably masks it. Probably worth testing to see.
| (this.enrolledInAppendRandomExperiment_ = | ||
| trials && trials.includes('amp-live-list-random')) | ||
| ); | ||
| } |
There was a problem hiding this comment.
:) This might need to be moved back (outside of the outer conditional) to address the case where ampdoc.isVisible() == false at the time register() is invoked. I think this could happen if a user switches to another tab immediately after opening an AMP page.
Yea this class's control flow is not the cleanest.
|
Hi @kristoferbaxter @choumx is there a reason this PR is held up? This was needed in prod by now. |
* Use a random number as a URL parameter for live list refreshes * JsonObject casting required * Is a dict * Add amp-live-list-random origin trial to gate adding a random identifier to each update of the live list * Changes based on PR feedback * Guard origin trial check in tri-state boolean * Add dep-check allowance * moved dep-check to correct segment * Only check for origin trial if the list is enabled
* Use a random number as a URL parameter for live list refreshes * JsonObject casting required * Is a dict * Add amp-live-list-random origin trial to gate adding a random identifier to each update of the live list * Changes based on PR feedback * Guard origin trial check in tri-state boolean * Add dep-check allowance * moved dep-check to correct segment * Only check for origin trial if the list is enabled
…pproject#25898) This reverts commit 0d52ea2.
Implement AMP Live List Cache Busting based on an Origin Trial. The Google AMP Cache has a bug where frequently updated content will not cause the distributed cache copy to correctly respond with the most up to date version of the document.
This issue is being addressed by the
wg-cachinggroup, but this Origin Trial is for domains who need more granular cached responses before the change to the Google AMP Cache is deployed.