Expose a public ConcurrentRequestCount property on ClusterState, like DestinationState #2223
Conversation
… DestinationState has We use different clusters for rolling out updates to our software. To support rollout we want each YARP server to monitor the outgoing clusters for remaining connections. When this drops to zero for a cluster, the YARP server communicates this to a central location and drops the reference to the ClusterState. Once all YARP servers have reported the cluster as drained (or a timeout occurs) we mark the deployment as complete. DestinationState has a public ConcurrentRequestCount property that works the same way, by exposing `ConcurrencyCounter.Value`. I didn't add the setter in this PR (DestinationState says it is only for testing) in the interests of a making the change smaller.
| public int ConcurrentRequestCount | ||
| { | ||
| get => ConcurrencyCounter.Value; | ||
| } |
There was a problem hiding this comment.
DestinationState.ConcurrentRequestCount for comparison:
reverse-proxy/src/ReverseProxy/Model/DestinationState.cs
Lines 61 to 71 in 20791da
There was a problem hiding this comment.
There's a race here where the count could be zero but a request is about to increment it. Any monitoring logic would need to wait a minimum amount of time in order to make sure all current requests are able to properly enter the pipeline.
There's also no way to monitor this value except polling which doesn't seem ideal. It seems like you don't need to know the actual value, just if it's still in use. Would a WeakHandle work just as well for that?
There was a problem hiding this comment.
Good point RE: the race condition.
Yep, polling isn't amazing. It will work fine for us in our context (the rate of poling we're thinking, the number of clusters we'll be watching at any time etc.) but I could see that being an issue generally.
You're right -- a weak reference would satisfy our use-case better.
|
Please don't submit PRs without first filing an issue to discuss the requirements and design. |
Sorry about that. I'm going to look into the weak reference thing for our use-case. That would still mean polling the outgoing resources. Do you want me to open an issue for destination/cluster draining monitoring in general? YARP could possibly help here e.g. by allowing you to listen for "draining complete" events, etc. |
|
Yes, please open an issue for the general scenario and we can look into possible solutions. |
|
Ok -- #2225 |
We use different clusters for rolling out updates to our software. To support rollout we want each YARP server to monitor the outgoing clusters for remaining connections. When this drops to zero for a cluster, the YARP server communicates this to a central location and drops the reference to the ClusterState. Once all YARP servers have reported the cluster as drained (or a timeout occurs) we mark the deployment as complete.
DestinationState has a public ConcurrentRequestCount property that works the same way, by exposing
ConcurrencyCounter.Value. I didn't add the setter in this PR (DestinationState says it is only for testing) in the interests of a making the change smaller.