*: add TTL support #25

GiedriusS · 2021-12-15T09:08:42Z

Add TTL support for keys. Implemented for our use case in https://github.com/thanos-io/thanos/ where cached data can potentially change over time. Thus, some kind of TTL mechanism is needed. Implemented it in the same fashion as https://github.com/mailgun/groupcache - deadline for keys is embedded in the value in the LRU cache. On retrieval, the deadline is checked and we are past it then we remove that key.

In HTTP the expiry date is exposed via an extra header. With gRPC, a new field has been added.

Note: this implies that users will have a somewhat synchronized time across nodes in the group. Otherwise, extra fetches might happen or the keys might not ever expire.

Add expiration time in preparation for the TTL feature.

TTLs are based on expiration in the LRU - during getting if the value is expired then it is removed from the LRU and we return nothing. This is needed to have short-lived values in a distributed system. To achieve this, expand the `valWithStat` struct with `expire time.Time`. Then, expand all of the related interfaces: - Codecs - `setLRUOnEvicted` - etc. Since the HTTP interface returns in the body raw bytes, send the expiration timestamp with headers. Expand the test in `http/http_test.go` to test whether the keys are being regenerated after the timestamp. Copy over a test from mailgun/groupcache for TTLs in the LRU.

*: add TTL functionality

dfinkel

First of all, Thanks for sending this PR!

With that said, I have some mixed feelings about supporting TTLs in Galaxycache in general, and the approach in this PR specifically.

First, my general opinion: Galaxycache works really well because it can treat everything as opaque and immutable. At times, expiration is better handled by changing the cache-key. (e.g. we (Vimeo's Core Services team) wrote this blog post about a scheme we've used for breaking time into epochs (per-key) instead of introducing TTLs to galaxycache.

However, there are still some places where TTLs would be useful, so I think it would be useful to have optional TTL support on some galaxies. (and we can come up with some advice on when to choose a TTL vs cleverly chosen keys)

With respect to this PR, unfortunately, I feel that there are too many breaking changes as-is. (I've noted a few in my comments)

In particular, I think that the Codec interface is a key public interface that would break a lot of client code if it changed much. However, I do like the change to RemoteGetter, I don't expect there to be many implementations of that outside this repository, so returning a new value is reasonable. (although we might want to make that new return-value a struct so we don't have to break the interface every time we add a new field)

I think an improvement would be to instead add a GetWithExpiry method to Galaxy and create an ExpiringBackendGetter interface for users to implement. (that has a GetWithExpiry method instead of a Get method). We'd have galaxycache type-switch so it prefers GetWithExpiry implementations.

With respect to lru.Cache, I think we should copy the existing implementation and add an lru.CacheWithExpiry, so galaxies that have BackendGetter implementations rather than ExpiringBackendGetter implementations wouldn't need to track the additional timestamps.

One thing that one of my teammates pointed out (@arodland ) is that the LRU supporting expiration should track expiring entries so upon eviction we don't evict an unexpired entry if there are any expired entries. (we might need a tree-map or skip-list of some sort to make that efficient for both insertion of arbitrary TTLs and eviction of the oldest one)

dfinkel · 2021-12-16T16:51:34Z

galaxycachepb/galaxycache.proto

@@ -17,6 +17,8 @@ limitations under the License.
 syntax = "proto3";

 package galaxycachepb;
+option go_package = "./galaxycachepb";


nit: this should be a fully-qualified go package path, so github.com/vimeo/galaxycache/galaxycachepb.

dfinkel · 2021-12-16T16:53:57Z

galaxycachepb/galaxycache.pb.go

@@ -1,212 +1,256 @@
+//
+//Copyright 2012 Google Inc.


Thanks for adding a license header, but as an auto-generated file, it really doesn't apply.
(also, the galaxycachepb gRPC interface should be copyright Vimeo inc. since that portion wasn't part of galaxycache)

dfinkel · 2021-12-16T16:57:03Z

galaxycachepb/galaxycache.proto

@@ -26,8 +28,9 @@ message GetRequest {
 message GetResponse {
  bytes value = 1;
  double minute_qps = 2;
+  int64 expire = 3;


Hmm, I think I'd rather use the Timestamp -- which translates to timestamppb in Go

dfinkel · 2021-12-16T16:58:25Z

go.sum

+github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
+github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=


this looks wrong; we don't need gogo protobuf for anything.
perhaps this needs a go mod tidy?

dfinkel · 2021-12-16T17:00:19Z

grpc/grpcserver.go

@@ -20,6 +20,7 @@ import (
 	"context"

 	gc "github.com/vimeo/galaxycache"
+	"github.com/vimeo/galaxycache/galaxycachepb"


this is a second import for the same thing as the one immediately below it.
We should either remove the pb alias and update the other references or use pb for the new reference.

Having two imports for the same thing with different names is confusing.

dfinkel · 2021-12-16T19:36:12Z

lru/lru.go

@@ -55,7 +57,7 @@ func New(maxEntries int) *Cache {
 }

 // Add adds a value to the cache.
-func (c *Cache) Add(key Key, value interface{}) {
+func (c *Cache) Add(key Key, value interface{}, expire time.Time) {


There are many uses of this lru.Cache type outside galaxycache. I'd rather not break the interface like this.
If we extend the existing type, it would be best to keep the signature of Add unchanged and add an AddExpiring method.

dfinkel · 2021-12-16T19:38:34Z

proto.sh

+#!/bin/bash
+protoc  --go-grpc_out=. --proto_path=galaxycachepb --go_out=galaxycachepb --go_opt=paths=source_relative galaxycache.proto


I think I'd rather have this as a //go:generate line in a generate.go in the galaxycachepb package.
proto.sh is a bit non-standard.

dfinkel · 2021-12-16T19:40:39Z

codec.go

+type ByteCodec struct {
+	bytes  []byte
+	expire time.Time
+}


This change breaks the common use-case of ByteCodec. (same with the other two public Codec implementations in this file)

This change makes it impossible to extract the value out for anyone outside this package.

dfinkel · 2021-12-16T19:41:15Z

codec.go

+	MarshalBinary() ([]byte, time.Time, error)
+	UnmarshalBinary(data []byte, expire time.Time) error


This is a very disruptive way to side-channel the expiration time around that breaks all existing Codec implementations as well as all BackendGetter implementations.

dfinkel · 2021-12-16T19:44:49Z

galaxycache.go

-	stats *keyStats
+	data   []byte
+	stats  *keyStats
+	expire time.Time


It's a bit memory-inefficient to add the expiration time to both to the LRU's value struct and the value we're storing inside the LRU.

GiedriusS · 2021-12-17T08:11:52Z

cc @akanshat

GiedriusS · 2022-01-18T10:48:29Z

Thank you for your thorough review, we will try the epochs approach and come back to you.

Upgrade internal interfaces for multi-key fetch: - Use multi-part HTTP request/response for retrieving data about multiple keys. Boundary is randomly generated by Go standard library so this seems like the best path; - Upgrade gRPC interface to have `repeat string Keys` instead of `string Key`. `key` has priority over `keys` but users should use `keys` either way. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

*: prepare for multi-key fetch

Switch to Go 1.17 && enable golangci-lint. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

.github: update workflows

* Add `ValueWithTTL` struct for grouping value with time.Time expiration time; * Update interfaces to use it; * Add stub for multiple key retrieval; * Fix golangci-lint errors. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

*: update interfaces

GiedriusS and others added 3 commits November 19, 2021 16:27

galaxycachepb: extend proto with expiration time

aeb97c6

Add expiration time in preparation for the TTL feature.

Merge pull request #1 from thanos-community/add_ttl

ab6f170

*: add TTL functionality

dfinkel requested changes Dec 16, 2021

View reviewed changes

GiedriusS mentioned this pull request Jan 6, 2022

Next steps with groupcache (galaxycache) thanos-io/thanos#5037

Open

7 tasks

GiedriusS and others added 6 commits June 21, 2022 10:14

Merge pull request #2 from thanos-community/prepare_for_multi_key_fetch

1b81014

*: prepare for multi-key fetch

.github: update workflows

6a89f51

Switch to Go 1.17 && enable golangci-lint. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

Merge pull request #3 from thanos-community/add_golangci_lint

e89e7e2

.github: update workflows

*: update interfaces

9b92dfa

* Add `ValueWithTTL` struct for grouping value with time.Time expiration time; * Update interfaces to use it; * Add stub for multiple key retrieval; * Fix golangci-lint errors. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

Merge pull request #4 from thanos-community/upgrade_interfaces

323e89e

*: update interfaces

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add TTL support #25

*: add TTL support #25

GiedriusS commented Dec 15, 2021

dfinkel left a comment

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

dfinkel Dec 16, 2021

GiedriusS commented Dec 17, 2021

GiedriusS commented Jan 18, 2022

		github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
		github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=

		#!/bin/bash
		protoc --go-grpc_out=. --proto_path=galaxycachepb --go_out=galaxycachepb --go_opt=paths=source_relative galaxycache.proto

		MarshalBinary() ([]byte, time.Time, error)
		UnmarshalBinary(data []byte, expire time.Time) error

*: add TTL support #25

Are you sure you want to change the base?

*: add TTL support #25

Conversation

GiedriusS commented Dec 15, 2021

dfinkel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiedriusS commented Dec 17, 2021

GiedriusS commented Jan 18, 2022