-
Notifications
You must be signed in to change notification settings - Fork 555
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
Editions crossword cryptic and quick data #27604
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.
Looks good! Couple of questions.
crosswords: EditionsCrosswordRenderingDataModel, | ||
)(implicit request: RequestHeader): Future[Result] = { | ||
val json = EditionsCrosswordRenderingDataModel.toJson(crosswords) | ||
post(ws, json, Configuration.rendering.articleBaseURL + "/EditionsCrossword", CacheTime.Facia) |
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.
Why CacheTime.Facia
?
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.
All of the other post requests in the DotcomRenderingService are using this cache time including for articles. Its confusingly named, maybe we can look at reviewing this separately.
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 think generally post requests for articles are using a different cache time based on their metadata:
post(ws, json, Configuration.rendering.articleBaseURL + path, page.metadata.cacheTime) |
post(ws, json, Configuration.rendering.interactiveBaseURL + "/Interactive", page.metadata.cacheTime, 4.seconds) |
with CacheTime.Default
as the fallback:
frontend/common/app/model/meta.scala
Line 326 in c7b403b
cacheTime: CacheTime = CacheTime.Default, |
?
Post requests for fronts and tag pages are using CacheTime.Facia
, but that's likely correct because they're examples of "facia" content. I agree that probably we should review getMedia
, getImageContent
and getGallery
's usage of CacheTime.Facia
; have captured this in #27628.
Given this, I don't think it's what we want to use here. Perhaps CacheTime.Default
is the correct choice if we don't have a specific, different cache setting for these pages? @SiAdcock @arelra any thoughts?
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.
Thanks. I didn't think about this much and just copied what was being done for similar use cases. What about 50 minutes surrogate and 5 minutes for client cache arbitrarily. This is a simple optimisation unlikely to effect users too much unless that are hitting crosswords in press reader at midnight
Seen on ADMIN-PROD (merged by @DanielCliftonGuardian 13 minutes and 41 seconds ago)
|
Seen on FRONTS-PROD (merged by @DanielCliftonGuardian 14 minutes and 1 second ago)
|
What is the value of this and can you measure success?
This PR adds functionality to the Crosswords Controller to send JSON data for the latest cryptic and quick crosswords to DCR
What does this change?
Resolves guardian/dotcom-rendering#12799