fix(cache) Don't cache failed calls to resourceStore

[lahabana]

We were caching errors to the resourceStore in the XDS cache
This was causing problems when the resourceStore was returning an error
we wouldn't attempt to get a value until the item was purged from the cache.
We now never cache errors, thus ensuring we recover quickly from resourceStore errors

Signed-off-by: Charly Molter charly@koyeb.com

[jakubdyszkiewicz]

that's a good one, thanks for introducing the test

[jakubdyszkiewicz]

What do you think about defer c.onceMap.Delete(mesh) as a first line?

Could you also do the same change in pkg/xds/cache/cla/cache.go?

[lahabana]

Both sound like good suggestions. Updating this

[lahabana]

@jakubdyszkiewicz there was a lot of redundancy between these 2 implementations I've refactored a bit to simplify and added an "error" result in the metric.

[jakubdyszkiewicz]

@lobkovilya could you check the refactor?

[lobkovilya]

Yeah, there was duplication indeed, but it's quite problematic to introduce a well-understandable and reusable abstraction here. I don't mind going with this once.Cache abstraction, but we have to do something with the interface because it's really unclear what Get does.

Maybe we can introduce new types:

type Key string
type Value interface{}
type Refresher func(context.Context, Key) (Value, error)

and rename Get to GetOrRefresh so it will look like:

func (c *Cache) GetOrRefresh(ctx context.Context, key Key, refresher Refresher) (Value, error)

and document this method with a comment something like:

// GetOrRefresh method returns cached Value if it was created within the last `expirationTime` time period.
// If the Value is older than `expirationTime` then Refresher will be launched and will update the Value.
// Regardless of the number of goroutines that have run into the expired Value, Refresher will be called only
// once, all goroutines will be blocked until the new Value returned by Refresher.
func (c *Cache) GetOrRefresh(ctx context.Context, k Key, r Refresher) (Value, error) {

WDYT?

[lahabana]

Sounds good!
How about GetOrCompute() as Refresh sounds like it will only work when the item has been in the cache?

For the Key what's the motivation for adding the subtype?

[lahabana]

The main reason for questioning the introduction of subtypes is that the underlying lib go-cache uses string as key and interface{} as value.

[lahabana]

In the end went for GetOrRetrieve and didn't add subtypes for key and value. Let me know what you think.

[lobkovilya]

I like GetOrRetrieve 👍

Subtypes help to make the signature more readable. If you define it like:

type FetcherFunc func(context.Context, string) (interface{}, error)

then it's not clear that FetcherFunc takes key as a parameter and returns value.

Also, interface{} is used twice - as a return value of GetOrRetrieve and as a return value of FetcherFunc. I think it makes sense to show that those interface{} are the same type. For example, if in the future we would like to introduce a new type for Value.

But I don't mind going with just string and interface{}

[lahabana]

@Mergifyio update

[mergify]

Command update: success

Branch has been successfully updated

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ lahabana
❌ mergify[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[jakubdyszkiewicz]

same thing fails on master, I don't want to block this PR from merging