Move WatchCache to pkg/storage and make it private #12848
Conversation
|
GCE e2e build/test passed for commit ffda1b2ffbd9427bdab06ff93be2034631065b8f. |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
ffda1b2 to
7974824
Compare
|
GCE e2e build/test failed for commit 7974824e5adde4b9298e27c9f1a53ed1e187e965. |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
7974824 to
4cdd35b
Compare
|
GCE e2e build/test passed for commit 4cdd35b8d2f8c0058b8873d0b24b1b2f9a98a73f. |
|
Can you justify this in the pr description? I guess I'm missing some context. I prefer a local hack to a global change at the interface level. Something like thread safe store should really not have to care about resource version, it should just be a locked hash map. Everyone who wants to use a locked cached would have to deal with this etcd style vector clock notation. |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
4cdd35b to
90732b6
Compare
|
The purpose of this PR is: However to do (1) I need to change the signature of Store::Replace() function to take resourceVersion as an additional argument (we need it for correctness in WatchCache). |
|
@bprashanth - I extended the PR description with explanation of what I'm doing. PTAL |
|
GCE e2e build/test passed for commit 90732b6b04f806ec89f0cb2519574819335e7d2e. |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
90732b6 to
6dba23b
Compare
|
@bprashanth - friendly ping - PTAL cc @lavalamp [for opinion about it] |
|
GCE e2e build/test passed for commit 6dba23b032c380d586d7a61c878e24b456ea8080. |
Why is this important? To do it, your're changing something that has nothing to do with the reflector or watch cache, to understand a watch concept. I want to be able to use something like thread-safe-store from the kubelet or networking package as a generic map with locks. Piping through a resource version for replace is not horrible, but doesn't make sense in that context. I'd rather avoid it, but I'm not hard against it. |
|
Thanks @bprashanth - I get your point (although I'm not fully convinced). @lavalamp - can you please make the final call on that? |
| @@ -107,7 +107,7 @@ func TestFIFO_addUpdate(t *testing.T) { | |||
| func TestFIFO_addReplace(t *testing.T) { | |||
| f := NewFIFO(testFifoObjectKeyFunc) | |||
| f.Add(mkFifoObj("foo", 10)) | |||
| f.Replace([]interface{}{mkFifoObj("foo", 15)}) | |||
| f.Replace([]interface{}{mkFifoObj("foo", 15)}, "0") | |||
There was a problem hiding this comment.
Nit: Real data probably would have rv being >= the highest RV in the list. So if that 15 is supposed to be a resource version, then the "0" should probably be at least "15"...
There was a problem hiding this comment.
yup - sorry. Should be fixed now.
|
@bprashanth @wojtek-t I think we want to evolve these caches into things that understand our objects, eventually into write-through caches etc. So I think being more explicit about resource versions in these caches seems like a good change to me. |
|
I think I agree with @lavalamp - my thinking is that in the fullness of time all those caches will be caches for "runtime.Object", which is already some dependence on resource versions. So I think it makes sense to proceed with that PR. |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
6dba23b to
f626282
Compare
|
@bprashanth @lavalamp - PTAL |
|
GCE e2e build/test failed for commit f62628288b17cc1e7e70468b89c6df4cb1ed1cfb. |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
f626282 to
ec03f44
Compare
|
GCE e2e build/test passed for commit ec03f44dc3cbd6fca6db7dc048cc3ad19eda8873. |
|
@lavalamp / @bprashanth - friendly ping |
k8s-github-robot
added
the
size/L
|
Labelling this PR as size/L |
| @@ -1,86 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Why is this removal part of this change?
There was a problem hiding this comment.
The poller is not used anywhere in the code.
However, it uses Replace method, which I would need to update, so I decided to just remove it.
|
I am fine with this, just a question + nit |
|
@lavalamp - PTAL |
|
@lavalamp - friendly ping |
wojtek-t
force-pushed
the
private_watch_cache
branch
from
ec03f44 to
d318b22
Compare
|
GCE e2e build/test passed for commit d318b22. |
|
@lavalamp - PTAL |
|
@lavalamp - friendly ping :) |
1 similar comment
|
@lavalamp - friendly ping :) |
|
LGTM |
lavalamp
added
the
lgtm
|
Sorry for delay! |
|
@k8s-bot test this [testing build queue, sorry for the noise] |
|
GCE e2e build/test failed for commit d318b22. |
|
Test failure is a flake: @k8s-bot test this please |
|
GCE e2e build/test failed for commit d318b22. |
|
Next flake: @k8s-bot test this please |
|
GCE e2e build/test passed for commit d318b22. |
|
@k8s-bot test this [testing build queue, sorry for the noise] |
|
GCE e2e build/test passed for commit d318b22. |
|
Automatic merge from SubmitQueue |
Auto commit by PR queue bot
This is a follow-up PR from discussion in #12721
The purpose of this PR is:
(1) change the reflector.go so that it doesn't have any reference to WatchCache
(2) make WatchCache to be a private class in pkg/storage (in fact it's just implementation detail)
However to do (1) I need to change the signature of Store::Replace() function to take resourceVersion as an additional argument (we need it for correctness in WatchCache).
I think this change is fine, because we use the store implementations together with Reflector (which already has a notion of resourceVersion) - the only places where we need to explicitly pass the some resource version are tests - I didn't have to modify any "production" code.
@bprashanth @timothysc