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

Refactor cache package and interfaces to something intentional #452

Closed
56quarters opened this issue Dec 18, 2023 · 0 comments
Closed

Refactor cache package and interfaces to something intentional #452

56quarters opened this issue Dec 18, 2023 · 0 comments

Comments

@56quarters
Copy link
Contributor

56quarters commented Dec 18, 2023

The current cache package is a mix of things that came from multiple packages with varying levels of abstraction in Thanos. We didn't really clean them up or decide what an appropriate cache interface would look like when removing the dependency on Thanos and copying code into dskit. Let's do that now.

I propose, in roughly the following order:

56quarters added a commit that referenced this issue Dec 19, 2023
Instead of returning the `RemoteCacheClient` interface from their
constructors, return the actual type of the client. This lets callers
decide how they want to use the clients and not be restricted to only
the methods supported by the interface.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Dec 19, 2023
Instead of returning the `RemoteCacheClient` interface from their
constructors, return the actual type of the client. This lets callers
decide how they want to use the clients and not be restricted to only
the methods supported by the interface.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Dec 19, 2023
Instead of returning the `RemoteCacheClient` interface from their
constructors, return the actual type of the client. This lets callers
decide how they want to use the clients and not be restricted to only
the methods supported by the interface.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Dec 20, 2023
Instead of returning the `RemoteCacheClient` interface from their
constructors, return the actual type of the client. This lets callers
decide how they want to use the clients and not be restricted to only
the methods supported by the interface.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Dec 20, 2023
Instead of returning the `RemoteCacheClient` interface from their
constructors, return the actual type of the client. This lets callers
decide how they want to use the clients and not be restricted to only
the methods supported by the interface.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 10, 2024
Move tracking of keys requested and hits from the `remoteCache` to each
respective cache client (Memcached and Redis). This is part of a refactor
that will eventually remove `remoteCache` since it doesn't provide enough
value to justify itself.

Related #452
56quarters added a commit that referenced this issue Jan 10, 2024
Move tracking of keys requested and hits from the `remoteCache` to each
respective cache client (Memcached and Redis). This is part of a refactor
that will eventually remove `remoteCache` since it doesn't provide enough
value to justify itself.

Related #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 10, 2024
Move tracking of keys requested and hits from the `remoteCache` to each
respective cache client (Memcached and Redis). This is part of a refactor
that will eventually remove `remoteCache` since it doesn't provide enough
value to justify itself.

Related #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 10, 2024
Move tracking of keys requested and hits from the `remoteCache` to each
respective cache client (Memcached and Redis). This is part of a refactor
that will eventually remove `remoteCache` since it doesn't provide enough
value to justify itself.

Related #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 11, 2024
Move tracking of keys requested and hits from the `remoteCache` to each
respective cache client (Memcached and Redis). This is part of a refactor
that will eventually remove `remoteCache` since it doesn't provide enough
value to justify itself.

Related #452
56quarters added a commit that referenced this issue Jan 11, 2024
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>
56quarters added a commit that referenced this issue Jan 16, 2024
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>
56quarters added a commit that referenced this issue Jan 17, 2024
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>
56quarters added a commit that referenced this issue Jan 29, 2024
Strip down and export the `remoteCache` struct to only act as an adapter
between the two cache interfaces in dskit: `Cache` and `RemoteCacheClient`.

This change also removes `MemcachedCache` and `RedisCache` since they were
only wrappers around the `remoteCache` struct and not referenced outside of
dskit itself.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 29, 2024
Strip down and export the `remoteCache` struct to only act as an adapter
between the two cache interfaces in dskit: `Cache` and `RemoteCacheClient`.

This change also removes `MemcachedCache` and `RedisCache` since they were
only wrappers around the `remoteCache` struct and not referenced outside of
dskit itself.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 29, 2024
Strip down and export the `remoteCache` struct to only act as an adapter
between the two cache interfaces in dskit: `Cache` and `RemoteCacheClient`.

This change also removes `MemcachedCache` and `RedisCache` since they were
only wrappers around the `remoteCache` struct and not referenced outside of
dskit itself.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this issue Jan 30, 2024
Strip down and export the `remoteCache` struct to only act as an adapter
between the two cache interfaces in dskit: `Cache` and `RemoteCacheClient`.

This change also removes `MemcachedCache` and `RedisCache` since they were
only wrappers around the `remoteCache` struct and not referenced outside of
dskit itself.

Part of #452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit to grafana/mimir that referenced this issue May 7, 2024
Specifically, this change pulls in grafana/dskit#520
which renames `cache.Cache` methods. Current uses have been updated to match
new names without any refactoring.

See grafana/dskit#452

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit to grafana/mimir that referenced this issue May 7, 2024
Specifically, this change pulls in grafana/dskit#520
which renames `cache.Cache` methods. Current uses have been updated to match
new names without any refactoring.

See grafana/dskit#452

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
Projects
None yet
Development

No branches or pull requests

1 participant