rls: move the data cache implementation into the rls package #5060
Conversation
balancer/rls/internal/cache.go
Outdated
| // callback provided by the LB policy to be notified when the backoff timer | ||
| // expires. This will trigger a new picker to be returned to gRPC, to force | ||
| // queued up RPCs to be retried. | ||
| callback func() |
There was a problem hiding this comment.
Can this be done along with timer via time.AfterFunc()?
I don't see where this actually does anything (yet?).
There was a problem hiding this comment.
This callback needs to be invoked not only when the timer expires, but also when the LB policy explicitly resets the backoff state. This happens when the control channel moves back to READY and the LB policy invokes the resetBackoffState method on the cache.
There was a problem hiding this comment.
Update comment to match? It says it's invoked when the timer expires but not anything else.
There was a problem hiding this comment.
Actually what I said was wrong. This callback is only invoked when the backoff timer expires. And it is invoked via a time.AfterFunc() which I just now changed to directly call the method on the LB policy which sends a new picker. See: https://github.com/easwars/grpc-go/blob/rls_master/balancer/rls/internal/picker.go#L329
I think at some point in time, I was using the same field for eviction callback and backoff timer expiry (not able to correctly recollect). But for some reason, I needed to be able to pass different callbacks. But that is not the case anymore. So, I'm totally getting rid of this field.
Thanks.
balancer/rls/internal/cache.go
Outdated
| // evicted. This is important to the RLS LB policy which would send a new picker | ||
| // on the channel to re-process any RPCs queued as a result of this backoff | ||
| // timer. | ||
| func (dc *dataCache) resize(size int64) bool { |
There was a problem hiding this comment.
It might be a good idea to name the return value since it's not obvious (without reading the comment above) what it should mean.
There was a problem hiding this comment.
Done.
updatePicker was the best name i could think of for the return value. Let me know if you have a better one.
There was a problem hiding this comment.
I was thinking backoffCanceled like the existing variable name, but either way.
- rename eviction callback in the cache entry - rename targetSize to maxSize in the cache entry - use named result parameters for certain dataCache methods - directly initialize childPolicyWrapper state - use base.ErrPicker instead of a new lamePicker type
|
Thanks for the review ! |
- remove `callback` field from `backoffState`
| return e.Value.(cacheKey) | ||
| } | ||
|
|
||
| func (l *lru) iterateAndRun(f func(cacheKey)) { |
There was a problem hiding this comment.
You might want to document what this function can and cannot be used for. For instance, deletions of the current key from l.ll are allowed, but reordering is not, and deleting the element immediately after the current key would be very bad.
balancer/rls/internal/cache_test.go
Outdated
| {path: "0", keys: "0"}, | ||
| {path: "1", keys: "1"}, | ||
| {path: "2", keys: "2"}, | ||
| {path: "3", keys: "3"}, | ||
| {path: "4", keys: "4"}, |
There was a problem hiding this comment.
Can you change the keys to "a"-"e" just to make sure there are differences between them and the paths?
Summary of changes:
balancer/rls/internal/cache/cache.gotobalancer/rls/internal/cache.gochildPolicyWrapperimplementation here since it is referenced by the data cacheOther changes to the data cache implementation:
READYRELEASE NOTES: n/a