Don't request all the blocks for liveblog main page #13566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| def withParser: Parser[Unit] = "with:" ^^ { _ => () } | ||
| def block: Parser[Unit] = "block-" ^^ { _ => () } | ||
| def id: Parser[String] = "[a-zA-Z0-9]+".r | ||
| def blockId = block ~> id |
There was a problem hiding this comment.
Would it make sense to make those private if they are not supposed to be exposed outside of this object?
|
Thanks for doing this @johnduffell |
|
thanks for picking through that one @TBonnin it can't have been fun, and more reviews are better. I did a lot of things I wasn't 100% happy with the result, I feel like the code ended up slightly more complicated in the end. I've done the updates you suggested, thanks |
|
I'm going to take a look at it now. |
| @@ -167,7 +194,7 @@ class ArticleController extends Controller with RendersItemResponse with Logging | |||
| .showReferences("all") | |||
| .showAtoms("all") | |||
|
|
|||
| val capiItemWithBlocks = if (blocks) capiItem.showBlocks("body") else capiItem | |||
| val capiItemWithBlocks = range.map(r => capiItem.showBlocks(r.query.map(_.mkString(",")).getOrElse("body"))).getOrElse(capiItem) | |||
There was a problem hiding this comment.
Maybe split this line up for readability?
|
Good stuff! I hope all this work pays off in bandwidth savings! I'm going to work this afternoon on adding the support for "blocks created/modified since" that we discussed. |
|
ok I've managed to get the tests passing, can anyone take a look and give me a +1 please? Next stage after this is to increase the cache time for "updates since block X" requests where block X isn't the latest block. That should leave only one html page and one json page with a 5 second cache time |
|
after that change all the perf updates should be complete, then I can start making it use published-since to get edits as well as new blocks and also fix the timeline to update again. |
|
👍 thank you very much @johnduffell |
# Conflicts: # article/app/controllers/ArticleController.scala
rtyley
added a commit
that referenced
this pull request
Aug 3, 2022
`LiveBlogCurrentPage.findPageWithBlock()` is responsible for making sure that when a user clicks on a live-blog permalink to a **block**, eg: https://www.theguardian.com/politics/live/2022/aug/01/tory-leadership-race-rishi-sunak-lizz-truss-vote-keir-starmer-uk-politics-live?page=with:block-62e7e89d8f08730a1f7f5579#block-62e7e89d8f08730a1f7f5579 ...the user is taken to the correct 'page' of the live blog - page 3 of 5 in the example above, for instance. The code was was first introduced in January 2016 with #11700, subsequently updated with #13566, etc. The existing code compiles without warnings under Scala 2.12, but the Scala 2.13 compiler gives a warning that not _all_ conceivable cases in the pattern-match are handled: ``` frontend/common/app/model/LiveBlogCurrentPage.scala:190:12: match may not be exhaustive. [warn] It would fail on the following inputs: List(_), Nil [warn] .map { [warn] ^ ``` In this particular case, the warning's unnecessary - the preceding `.sliding(3)`: https://github.com/guardian/frontend/blob/f90c8a58e6f0941c96392d14fe27e61a4dbaf25b/common/app/model/LiveBlogCurrentPage.scala#L188 ...means that the List supplied to the case match expression will _always_ have 3 elements, and that's precisely the case that's handled - but there's no way for the compiler to know that - so we as devs need to do _something_ to make the warning go away! See also: * The Scala 2.13 upgrade PR: #25190
rtyley
added a commit
that referenced
this pull request
Aug 3, 2022
`LiveBlogCurrentPage.findPageWithBlock()` is responsible for making sure that when a user clicks on a live-blog permalink to a **block**, eg: https://www.theguardian.com/politics/live/2022/aug/01/tory-leadership-race-rishi-sunak-lizz-truss-vote-keir-starmer-uk-politics-live?page=with:block-62e7e89d8f08730a1f7f5579#block-62e7e89d8f08730a1f7f5579 ...the user is taken to the correct 'page' of the live blog - page 3 of 5 in the example above, for instance. The code was was first introduced in January 2016 with #11700, subsequently updated with #13566, etc. The existing code compiles without warnings under Scala 2.12, but the Scala 2.13 compiler gives a warning that not _all_ conceivable cases in the pattern-match are handled: ``` frontend/common/app/model/LiveBlogCurrentPage.scala:190:12: match may not be exhaustive. [warn] It would fail on the following inputs: List(_), Nil [warn] .map { [warn] ^ ``` In this particular case, the warning's unnecessary - the preceding `.sliding(3)`: https://github.com/guardian/frontend/blob/f90c8a58e6f0941c96392d14fe27e61a4dbaf25b/common/app/model/LiveBlogCurrentPage.scala#L188 ...means that the `List` supplied to the case match expression will _always_ have 3 elements, and that's precisely the case that is handled - but there's no way for the compiler to know that - so we as devs need to do _something_ to make the warning go away! Some options: * Use the @unchecked annotation (see https://www.scala-lang.org/api/2.12.7/scala/unchecked.html) This is an option of last resort - turning off compiler checks leaves us exposed to runtime errors and is generally a bad idea! * Use `collect()` rather than `map()` - this accepts a partial function, rather than a total one, so the compiler error will go away. https://www.scala-lang.org/api/2.12.7/scala/collection/immutable/Seq.html#collectFirst[B](pf:PartialFunction[A,B]):Option[B] * Refactor so that the code no longer needs to assume that a `List` type (which can have any length) has length `3`. In the end I decided to go with the refactor, because there were a few things about the existing code that could be tweaked: * Unnecessary work: The method was creating a `LiveBlogCurrentPage` for _every_ page in the liveblog, even though it only ever needed one (the single page that the block actually exists on!), and would eventually throw away the rest. * The logic around padding the front & end of the `pages` List with two `None` entries, to allow extracting the `newer` & `older` pages, totally worked but added an extra step to the code (and therefore a bit of complexity for humans to understand: "what are endedPages?") and could be replaced by just incrementing/decrementing the page index for the one `LiveBlogCurrentPage` we're now creating. * Various variables could be inlined to the point of creation on the `LiveBlogCurrentPage` case class. By using named parameters in constructing the case class, no clarity is lost, and the code is more concise. See also: * The Scala 2.13 upgrade PR: #25190
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the CAPI query for liveblog page 1 (the live page) and the ajax "get updates" call to only retrieve the necessary blocks, rather than the whole liveblog.
This should save a lot of bandwidth as liveblogs get bigger, and also reduce the CPU usage both in CAPI and frontend. We noticed things were particularly bad when there were many big liveblogs running after brexit. This is a problem particularly for those two endpoints because they only have a 5 second cache time. Archive pages of a live blog cache for a few minutes.
This is part 2 of the series - part 1 was #13458
@TBonnin @cb372 @JustinPinner any comments or +1s please!