Update resource cache after update/create from Dependent Resource #953
Conversation
csviri
changed the title
| @@ -39,4 +40,8 @@ public void stop() { | |||
| super.stop(); | |||
| manager().stop(); | |||
| } | |||
|
|
|||
| public void populateCacheUpdatedResource(R resource) { | |||
| cache.put(ResourceID.fromResource(resource), resource); | |||
There was a problem hiding this comment.
Based on fabric8io/kubernetes-client#3654 (comment), I'm not sure we should be modifying the informer's cache :(
Another thing is that this won't be possible in 6.0 anymore so we need to think about an alternative, in particular in the light of: fabric8io/kubernetes-client#3616. So we might have to think about re-adding our own caching layer sooner than later…
There was a problem hiding this comment.
Yes, the plan is to do it with the separate cache if the informers will be modified for that. Not sure if we can get away with this until that time. It's not 100% correct, but the chance to have there a race condition is very close to zero
There was a problem hiding this comment.
An alternative is to wait for the event that the cache received the resource with the resource version.
There was a problem hiding this comment.
That would mean not the update/delete would propagate the cache but the getResource() will be kinda "synchronous"
There was a problem hiding this comment.
Solved it in a different way, pls see updated PR. (Will add unit tests soon)
# Conflicts: # operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
|
Kudos, SonarCloud Quality Gate passed! |
|
I'm thinking that this work should be integrated in the |
|
The thing is that this is not needed when you work directly with resources, since when a resource is created by the client a new object is returned that users can work with further. So it us really useful for dependent resources. Since the getResource() is called eventually by other dependent resource. Plan is to put it there yes, after the cache layer will be separated from informers, as mentioned by @shawkins; for an upcoming version of informers. So for now I would just merge this, and create a followup issue. Putting this updated resource now to cache of the informers has race condition issues, that are not nicely solvable now. |
|
just a remark this issue I actually plan to put into informer event source: #870 It's kinda similar to this. So we can revisit also there. Not sure we need this to MVP though, maybe right after? |
|
Hmm, actually you are right, can take a look, and move the temporal layer to the event source. now this impl looks quite different then on begging. Maybe would be able to implement also the mentioned issue in a simple way. |
|
this PR is replaced by: #965 |
No description provided.