Use WatchListXXXForAllNamespacesAsync to list Kubernetes resources and refactor WatchAsync#3024
Use WatchListXXXForAllNamespacesAsync to list Kubernetes resources and refactor WatchAsync#3024Xulei-NL wants to merge 15 commits into
WatchListXXXForAllNamespacesAsync to list Kubernetes resources and refactor WatchAsync#3024Conversation
|
The PR for the issue #3008. |
|
@dotnet-policy-service agree |
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
…s' into refactor/asyncwatch-k8s-resources
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
…s' into refactor/asyncwatch-k8s-resources # Conflicts: # src/Kubernetes.Controller/Client/ResourceInformer.cs
| // Used as an atomic boolean to ensure the re-connect cancellation path is triggered only once. | ||
| // 0 means cancellation has not been requested; 1 means cancellation has already been requested. | ||
| // Access only through Interlocked operations. | ||
| public int CancellationRequested; |
There was a problem hiding this comment.
Why is this needed? You're only calling .Cancel() on the CTS, there's no harm in doing so multiple times
There was a problem hiding this comment.
Dear @MihaZupan ,
indeed calling .Cancel() many times has no harm in the timer. The code currently on the master branch uses lastEventUtc = DateTime.MaxValue; to make sure the cancellation happened once. See https://github.com/dotnet/yarp/pull/3024/changes/BASE..1462c1a1f4822994206edbb8084a65fe8bb93be0#diff-d890e91050758eee4329344d227734ac5b1d3e99f2e27a9dead7c564f206c8acL339.
I regarded this PR as a refactoring PR so I wanted to make sure the log below was only printed once:
Logger.LogDebug(
EventId(EventType.DisposingToReconnect),
"Disposing watcher for {ResourceType} to cause reconnect.",
typeof(TResource).Name);I'm also totally fine if we remove the public int CancellationRequested. Do you think we need to remove it?
There was a problem hiding this comment.
Yeah I'd remove it. If your cancellation took 45 seconds to kick in your process is very dead anyway
There was a problem hiding this comment.
Clear. I have removed it and meanwhile refactored the comments. Will request you a new review now.
2 participants
This PR
WatchListXXXForAllNamespaceswithWatchListXXXForAllNamespacesAsync. For instaceWatchListEndpointsForAllNamespaceswithWatchListEndpointsForAllNamespacesAsync. If so, we could get watch results asynchrounously.WatchAsyncto handle returnedIAsyncEnumerable(<WatchEventType, TResource>)in step 1. In this way we could removeTaskCompletionSourcewhich simulates a Task and rely onawait foreach.Appreciate every feedback!
Closes #3008