Support ETCD Prefix KV Retrievals and Watches #4333
Conversation
src/cluster/kv/types.go
Outdated
| C() <-chan struct{} | ||
| Get() map[string]Value | ||
| Close() |
There was a problem hiding this comment.
Is the client expected to receive notification from the channel and call Get() for the updated value? If so, why not pass the relavant key via channel so we can save an additional call?
There was a problem hiding this comment.
yeah the expectation is to invoke a Get() once the channel receives an update. It will return the entire map of values and it will be up to the application to decide which ones are new vs deleted etc. This was done to eliminate cases where updates are missed on non-blocking channel-full enqueues from ETCD client into the xwatch.Watchable interface. This also keeps the existing interfaces intact since breaking that would break at many places since this store is heavily used.
src/cluster/kv/etcd/store.go
Outdated
| if len(values) == 0 { | ||
| // At deletion, just update the watch to nil. | ||
| return watchable.Update(nil) | ||
| } |
There was a problem hiding this comment.
if we do this, would we inadvertently remove keys that the client has yet to read?
for example:
- etcd receives two updates /pre/k1 => v1, /pre/k2 => v2
- updateForPrefix does its thing and updates the internal map. map is now {k1: v1, k2: v2}
- a signal should be sent to the client to handle updates, but let's say it does something "long" as part of its update and doesn't call "Get" right away.
- etcd receives a deletion event, /pre/k2 is now deleted.
- updateForPrefix does its thing and updates the internal map. if i'm reading this right, the map is now empty per this update call?
- the client finishes it's long task and calls "Get", but now, k1 is missing.
is this possible here?
There was a problem hiding this comment.
Good callout. The existing single-key-value KV store simply relies on the latest state of a key-value. For prefix-store we rely on the same semantics: For every key-value in the prefix range we will only rely on the latest state of each of the key-values. So if a key-value exists transiently and then goes away then we are ok to only rely on the state where it never existed.
There was a problem hiding this comment.
that makes sense--but in my example, I believe /pre/k1 should still exist because only /pre/k2 was deleted. am I thinking about this incorrectly?
There was a problem hiding this comment.
aah yes! you're right! The entire map will be deleted. It doesn't matter if the read was slow or not. Good catch! I've fixed that issue and added unit tests for various scenarios.
src/cluster/kv/types.go
Outdated
| GetForPrefix(prefix string) (map[string]Value, error) | ||
| WatchForPrefix(prefix string) (PrefixWatch, error) |
There was a problem hiding this comment.
non blocking: i think there's already a good amount of context that the operations here are for prefix based things, maybe calling these functions GetValues and Watch will suffice?
There was a problem hiding this comment.
:-) I wanted to do that but I'm reusing the same client struct that was used for satisfying the Store interface which already contains Get() and Watch().
What this PR does / why we need it:
At the moment we can only watch absolute paths individually in ETCD KV store. With this change, we create a PrefixStore that can retrieve and watch a prefix path. This PrefixStore only performs read operations of Get() and Watch(). Write operations like Set(), Delete() etc are not supported.
The fundamental basis of the change is to introduce this feature without breaking any existing exported interfaces.
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: