removing resync and updating some logs #292
Conversation
|
This is to further assist with issues we've seen. Based upon our deeper understanding of resync, it does not need to run at all - this will help remove a potential source of events. The kafkaCluster log is too high for the information it provides, and the resource events should show their resource version - which will help correlate to the state seen in yaml output of the resources. |
| @@ -74,33 +74,33 @@ void onStart(@Observes StartupEvent ev) { | |||
| // TODO: should we make the resync time configurable? | |||
|
|
|||
| kafkaSharedIndexInformer = | |||
| sharedInformerFactory.sharedIndexInformerFor(Kafka.class, KafkaList.class, operationContext, 60 * 1000L); | |||
| sharedInformerFactory.sharedIndexInformerFor(Kafka.class, KafkaList.class, operationContext, 0); | |||
There was a problem hiding this comment.
without the resync how does it work in case of lost events?
There was a problem hiding this comment.
I need to look more at what the behavior is on 5.0.0 with the 0 setting, but what they have changed the implementation into based upon fabric8io/kubernetes-client#2812 is that resync is purely in memory - it won't contact the api server at all.
Missed events are assumed to not be possible as long as a watch is active. Once a watch is closed a reconnect is performed. Once connected again the logic performs the necessary catch up - that is where we saw the delete bug, because the catch up logic did not update the index in that case.
There was a problem hiding this comment.
I don't know the fabric8 internal but when you say "resync is purely in memory" ... so it's resyncing with what if it doesn't contact the API server. My understanding was resyncing the in-memory cache contacting the API server.
There was a problem hiding this comment.
They made a mistake in 5.0.0 compared to the go client logic. fabric8 on the resync interval was performing a relist. They are now removing that based upon the linked issue. So the resync expectation is to simply generated update events for everything that exists in the cache - that's it. Since we are ignoring these events anyway (with the resource version difference checks) we don't need any resync.
There was a problem hiding this comment.
:-o ... so this fabric8 resync sounds like more dangerous than useful imho
There was a problem hiding this comment.
At this point, yes it is. We thought we were relying on behavior that was a mistake... So it's best to turn it off now.
(cherry picked from commit ea96db7)
No description provided.