[rid] Add lock on subscriptions - #1523
Conversation
|
I switched that one to draft. Running tests with the new proposed tool in interuss/monitoring#1519, 27 queries in // show those results: 
Things seems faster without latency, but as soon as it increase things explode, and that probably dues to similar reason to SCD: too much contentions. Trying again with minimum number of queries in //, 9. we get: 
Same problem, but with higher latency. Not sure how to decide between different tradeoff. With this PR, request can be processed faster (especially with light load), but there is a point (dependant on latency) when it's worse. |
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
Hum looking at those results, it seems like this change actually makes the performances (slightly) worse in real-world settings? Or am I missing something?
But if that is the case I don't think we would want to implement this?
No, I don't think the answer is that clear: it make performances worse when the // is too high, but better when it's not the case. The question is now what are those limits (in term of // queries) and the new benchmarking tool of @BenjaminPelletier can answer that: if it's increase the number of possibles queries in // before everything 'collapse', that probably better in general, even if it's collapse in a worst way. |
|
(Extra info: what is "usefull" in the current graph in the number of errors, we can perform more success full queries with this PR, especially with less queries in //. raw q/s and timeout are almost meaningless as lot of queries timeout :/) |
|
It's very challenging to figure out performance as there are many different things that could affect it and many different flag combinations. Here are some benchmarker tests I've run with flights consisting of creating an ISA and deleting that ISA on a 3x3 local DSS deployment (variations on this configuration). No subscriptions, default flags: 
8 subscriptions, default flags: 
Time-based notification index, 8 subscriptions (multiple runs to verify behavior): 
Time-based notification index, no subscriptions: 
It seems like this change has a substantial negative impact in cases where subscriptions are present, especially in the presence of the time-based notification index behavior which I'm expecting will be the main operating mode. Let's discuss further in the Tuesday meeting; I wonder if the time-based notification index by itself would resolve the underlying problem without the need for this change. I also wonder if there may be similar effects on the SCD side if a lock similar to this is used. |
|
Given the discussion from yesterday's InterUSS weekly meeting, #1523 (comment) and #1614 (comment), I think we can close this PR. |
3 participants
This PR adds a lock on RID's subscriptions when working on ISAs.
It fixes #1509 (or at least improves the situation).
This follows the same logic as the one in SCD, preventing (improving) cascades of transaction restarts observed with tracing using parameters / code in #1509 (comment)
However ISAs don't create an automatic subscription, so the case with a lot of subscriptions may not be relevant.
This has been verified locally by running tests as described previously (parameters + specific non-merged version of monitoring repo). Timeouts present before are gone.
Before:
After:
(Note that there is an unrelated error on subscriber notification)