Introduce support for per-call memory allocator for caching

[56quarters]

What this PR does:

This change adds the ability to pass a memory allocator to caching methods using one or more Option arguments. This allows the underlying client to use memory allocators better suited to their workloads than GC. This is only used by the Memcached client at the moment.

Which issue(s) this PR fixes:

See grafana/mimir#3772
See grafana/gomemcache#8

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

Checklist

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

[56quarters]

This approach currently uses a context.Context object to pass an Allocator instance to the various caching layers. I'm not sure if this is a hacky way of "passing optional parameters to functions". In my mind an allocator isn't really something you'd normally pass as an argument. Do people have thoughts or opinions on this?

[charleskorn]

This approach currently uses a context.Context object to pass an Allocator instance to the various caching layers. I'm not sure if this is a hacky way of "passing optional parameters to functions". In my mind an allocator isn't really something you'd normally pass as an argument. Do people have thoughts or opinions on this?

My 2c: I would prefer an Options-style parameter, like what you introduced in gomemcache, although I agree that it feels a bit weird.

Rationale:

As a consumer of this library, an Options-style parameter makes the functionality more discoverable, whereas using a context.Context, I'd need to do a bit more digging to find it. (Documentation might help mitigate this though.)

Using parameters would also help avoid annoying bugs - for example, imagine a scenario where we had a couple of different libraries that all expected the allocator to be passed in the context.Context, each with their own WithAllocator(ctx, allocator) method. It'd be really easy to accidentally use the wrong WithAllocator method, and it wouldn't be immediately obvious that you'd done this: instead, we'd just silently use the default allocator. With parameters or an Options-style parameter, either you've set the allocator (and type safety prevents any confusion), or you haven't.

[56quarters]

This approach currently uses a context.Context object to pass an Allocator instance to the various caching layers. I'm not sure if this is a hacky way of "passing optional parameters to functions". In my mind an allocator isn't really something you'd normally pass as an argument. Do people have thoughts or opinions on this?

My 2c: I would prefer an Options-style parameter, like what you introduced in gomemcache, although I agree that it feels a bit weird.

Rationale:

As a consumer of this library, an Options-style parameter makes the functionality more discoverable, whereas using a context.Context, I'd need to do a bit more digging to find it. (Documentation might help mitigate this though.)

Using parameters would also help avoid annoying bugs - for example, imagine a scenario where we had a couple of different libraries that all expected the allocator to be passed in the context.Context, each with their own WithAllocator(ctx, allocator) method. It'd be really easy to accidentally use the wrong WithAllocator method, and it wouldn't be immediately obvious that you'd done this: instead, we'd just silently use the default allocator. With parameters or an Options-style parameter, either you've set the allocator (and type safety prevents any confusion), or you haven't.

That's a great point: that would make the API much harder to use incorrectly. I'll work on that change tomorrow.

[charleskorn]

LGTM

[pracucci]

Good job, LGTM! I just left a nit.