Avoid Caffeine exceptions on missing values #56
Conversation
We want a way for our `Loading` object to indicate the value for a key is `Missing` (eg we are trying to load an S3 key that does not exist) without having to throw an exception - as Caffeine will noisily log almost every exception it sees, even if the client does not mind missing values, as is the case with the Facia Scala Client. We've introduced the `MissingOrETagged` sealed trait to allow us to represent that a fetch result may be either missing or found, returned with ETagged data. This is an alternative approach to the hacky solution in #32 . Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
|
@rtyley has published a preview version of this PR with release workflow run #108, based on commit cbc4304: 4.0.0-PREVIEW.suppress-caffeine-exceptions-by-allowing-fetch-results-to-indicate-missing-keys.2024-07-09T1701.cbc4304a Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the suppress-caffeine-exceptions-by-allowing-fetch-results-to-indicate-missing-keys branch, or use the GitHub CLI command: gh workflow run release.yml --ref suppress-caffeine-exceptions-by-allowing-fetch-results-to-indicate-missing-keys Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
rtyley
changed the title
Missing valuesThe `etag-caching` library has had a small change to its API with guardian/etag-caching#56, so we need make a small code change to adapt to it. # Background ETag Caching was originally introduced to Frontend with #26338.
| def get(key: K): Future[V] = read(key).map(_.result) | ||
|
|
||
| def get(key: K): Future[Option[V]] = read(key).map(_.toOption) |
There was a problem hiding this comment.
This is the only external API change in this PR.
Internally, you can see we are using the toOption field on the new MissingOrETagged type - we'll return None if the value is Missing. If we have a populated ETaggedData, we just return the value, because the end user won't care about about the ETag.
| def toOption: Option[T] | ||
| } | ||
|
|
||
| case object Missing extends MissingOrETagged[Nothing] { |
There was a problem hiding this comment.
Just like the None singleton that extends Option[Nothing], we take advantage here that in Scala Nothing extends all types, and so this Missing singleton is a valid value for any MissingOrETagged[+T].
rtyley
deleted the
suppress-caffeine-exceptions-by-allowing-fetch-results-to-indicate-missing-keys
branch
The `etag-caching` library has had a small change to its API with guardian/etag-caching#56, so we need make a small code change to adapt to it. ETag Caching was originally introduced to Frontend with #26338.
Why?
We want to use the
etag-cachinglibrary with Facia Scala Client (see guardian/facia-scala-client#287) so that Ophan can do more frequent querying of the Fronts API while reducing CPU & network consumption.However, the Facia Scala Client, in normal use, will frequently make requests for Collection JSON entries that don't exist in S3. Code already exists in Facia Scala Client to catch & suppress the exceptions when entires don't exist, but as we now want to now put fetching behind a Caffeine cache (ie with
etag-caching) we run into the problem that any exception during loading will be logged by Caffeine and in our case this makes excessive logging noise.To solve this problem, we now introduce a way for our
Loadingobject to indicate the value for a key isMissing(eg we are trying to load an S3 key that does not exist) without having to throw an exception. This is an alternative approach to the hacky solution I worked on in #32 !Change to external API of
ETagCacheBefore
After
The only code using the
etag-cachinglibrary at the moment is Frontend, we have a small PR to update Frontend to cope with the API change:etag-cachinglib with small API change for missing values frontend#27303Internal changes
The Caffeine cache is now storing values which are the new
MissingOrETaggedtype, rather than justETaggedData.We've introduced the
MissingOrETaggedsealed trait to allow us to represent that a fetch result may be eitherMissingor found, returned withETaggedData.See also: