Skip to content
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

Remove errors from set methods on RemoteCacheClient interface #466

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Jan 11, 2024

What this PR does:

This change does a few things:

  • Removes the error from the set method on the RemoteCacheClient interface. The only error that can be returned was an error when the async queue used for set operations was full. All other errors were logged and a counter incremented. Even for this error, it was special-cased and nil was returned instead. Thus, the interface included an error return value that could never actually happen.
  • Adds another method, SetMultiAsync to the interface for setting multiple key-value pairs at once. This was the only functionality that the remoteCache struct was providing beyond the cache clients it was wrapping. With this new method, there's no longer a reason to keep it around (will address in a follow-up PR).
  • Uses relative TTLs for Memcached items. Memcached supports using a relative TTL (e.g. 300 for an item that expires after 5 minutes) or absolute TTL (a particular UNIX timestamp). We were converting time.Duration objects to an absolute TTL for no reason.

Which issue(s) this PR fixes:

Part of #452

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

return c.client.Set(&memcache.Item{
Key: key,
Value: value,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old method was actually using the wrong var here, setting value from the argument to SetAsync instead of buf being passed to the closure.

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. I don't mind the error returned from SetAsync, especially because adding it back later might break linters and it's only an error. But maybe we'll cross that bridge when we come to it.

Found two outdated comments

cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
@56quarters
Copy link
Contributor Author

SGTM. I don't mind the error returned from SetAsync, especially because adding it back later might break linters and it's only an error. But maybe we'll cross that bridge when we come to it.

I'm OK removing the errors from RemoteCacheClient because we don't actually use that anywhere in Mimir except RemoteIndexCache in the store-gateway. Even there, the errors are logged instead of being returned (but again, they couldn't actually ever happen with the current implementations).

Found two outdated comments

Fixed, thank you.

This change does a few things:

* Removes the error from the set method on the `RemoteCacheClient`
  interface. The only error that can be returned was an error when
  the async queue used for `set` operations was full. All other errors
  were logged and a counter incremented. Even for this error, it was
  special-cased and `nil` was returned instead. Thus, the interface
  included an `error` return value that could never actually happen.
* Adds another method, `SetMultiAsync` to the interface for setting
  multiple key-value pairs at once. This was the only functionality
  that the `remoteCache` struct was providing beyond the cache clients
  it was wrapping. With this new method, there's no longer a reason
  to keep it around (will address in a follow-up PR).
* Uses relative TTLs for Memcached items. Memcached supports using
  a relative TTL (e.g. 300 for an item that expires after 5 minutes)
  or absolute TTL (a particular UNIX timestamp). We were converting
  `time.Duration` objects to an absolute TTL for no reason.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters force-pushed the 56quarters/cache-error-handling branch from e9cdec4 to 0c7ca84 Compare January 16, 2024 21:05
@56quarters 56quarters merged commit b9a439d into main Jan 17, 2024
3 checks passed
@56quarters 56quarters deleted the 56quarters/cache-error-handling branch January 17, 2024 14:43
@dimitarvdimitrov
Copy link
Contributor

I'm OK removing the errors from RemoteCacheClient because we don't actually use that anywhere in Mimir except RemoteIndexCache in the store-gateway

hopefully one day dskit becomes a real library, not just a sub-package of mimir :D

56quarters added a commit to grafana/mimir that referenced this pull request Jan 17, 2024
Specifically pulls in grafana/dskit#466 which changes the `RemoteCacheClient`
interface and Memcached/Redis implementations of it, removing an impossible
error return.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit to grafana/mimir that referenced this pull request Jan 17, 2024
Specifically pulls in grafana/dskit#466 which changes the `RemoteCacheClient`
interface and Memcached/Redis implementations of it, removing an impossible
error return.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants