-
Notifications
You must be signed in to change notification settings - Fork 554
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
Improve live blog bandwidth use #13458
Conversation
val allPagesJson = Seq( | ||
"timeline" -> timelineHtml, | ||
"numNewBlocks" -> newBlocks.size, | ||
"mostRecentBlockId" -> page.article.fields.blocks.headOption.map(block => s"block-${block.id}").getOrElse("block-9") |
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.
what is block-9
?
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.
just a block it definitely won't find in the list, that would only happen if a liveblog was somehow published without any blocks (which isn't possible). If we're looking since a certain block and it doesn't exist, we just return them all.
Having said all that, maybe it could be clearer. I'll take a look.
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.
oh and the 9 was a typo it was supposed to be 0, but block 9 is an area at glastonbury so I thought I'd leave it ;)
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.
what about block-does-not-exist
? 🎯
ok I've taken out the default value @TBonnin as it didn't make a lot of sense to force it |
is that a +1 for the whole PR or just the block change? |
It looks good to me, unless @TBonnin has another comment 👍 |
Sorry I forgot to give you a 👍 |
When a live blog is running, clients request updates every 5 seconds using an ajax url.
To do this, they ask for all blocks since the last block that was loaded onto the screen, and if the user reads those blocks (click toast or scroll up) then it uses the new latest block as the last block. This endpoint, like the live page itself, has a 5 second cache time regardless of which block is requested.
This causes a great deal of cache splitting, every extra block splits the cache again. With the 5 second cache time, this gets expensive. With multiple liveblogs running, if they're big, we're wasting resources.
This PR is the first (of probably 3) to solve the problem.
Part 1: this part: arguably a bug fix - make the clients always request from the last block they retrieved, and not wait for the user to read the block before using it. This will save the user repeatedly downloading the same blocks every 5 seconds, and possibly save us a little on not having to serve as many old blocks.
Part 2: I will make the cache time of anything other than the latest block (i.e. empty response) to be Very Long. This means requests for one block only (the latest) will be getting to the backend every 5 seconds, the others will all be left in the cache. The clients will only get one block at a time, so they'll have to poll again immediately after any successful update. This will take most of the requests off the backend. Note: if editorial add a block, and then delete it, we might get stuck in the past. The purger only clears theguardian.com, not api.nextgen. Actually, even now, deleting blocks would mess things up. Editing in general is basically not supported with auto upates. The timeline isn't auto updated either by the looks 😦
Part 3: finally 😊 use @cb372 's https://github.com/guardian/content-api/pull/1502 and only get the blocks we need for the endpoints that are only cached for 5 seconds. This will make the requests that we do process every 5 seconds much cheaper for us and capi.
@alexduf @TBonnin @JustinPinner comments comments comments!