APIGOV-33163 - new event kind handler structure#1059
Conversation
|
|
||
| cachedSeq := cv.sequence.GetSequence() | ||
| if serverSeq != cachedSeq { | ||
| // At start-up or agent restart, because of the status change event, the serverSeq will always be higher with 1 than the cachedSeq. |
There was a problem hiding this comment.
maybe just a comment thing for me. looks like this code is also called from validateAndRebuildCache. So this +1 tolerance is also processing every mid run reconnect as well, no?
There was a problem hiding this comment.
I think mid run reconnects would happen in case the watch controller context gets cancelled(SDK stream client timeout), which would mark the agent as stopped and still trigger a status change to Stopped
There was a problem hiding this comment.
I think this +1 check is not needed. Watch-controller agent status update will not raise events, so the sequence should be same
There was a problem hiding this comment.
hm... weird. I was permanently getting sequence mismatches(+1 difference) during testing and my assumption was that this was the reason. I guess I'll revert it then
| } | ||
| } | ||
|
|
||
| func (h *discoveryManagedApplication) ShouldHandle(ctx context.Context, event *proto.Event) bool { |
There was a problem hiding this comment.
Don't know if i'm seeing this correctly. Could you take another look at the ShouldHandle here (and the same pattern in discoveryaccessrequest.go:24, tracemanagedapplication.go:24, and traceaccessrequest.go:27)?
It returns false once h.cache.GetManagedApplication(event.Payload.Metadata.Id) finds an existing entry. But what about the DELETE action. Since Handle() never runs when ShouldHandle() returns false, it looks like once a resource is cached, later update and delete events for that same ID might not get processed by this handler anymore??? That's what i'm seeing. Just checking in case it's worth a second pass.. could be intentional, but wanted to double check it.
There was a problem hiding this comment.
hm... I was trying to not change the old behaviour for this code. I think we're not interested in any updates apart from the resource itself being there.
Regarding your statement for the DELETE action, you're right. good call
Rebasing onto main's v1alpha1->v1 model migration (432c8eb) moved AccessRequest out of v1alpha1, breaking this file's import.
4 participants
No description provided.