Evict the longer time stamp subscription #37025
Conversation
|
Review changes with |
| @@ -1128,6 +1128,15 @@ bool InteractionModelEngine::TrimFabricForSubscriptions(FabricIndex aFabricIndex | |||
| { | |||
| candidate = handler; | |||
| } | |||
| // The last report time of this handler is longer | |||
There was a problem hiding this comment.
Why is this the right logic? What is the actual behavior we are trying to implement?
If it's "evict the least newly created subscription", that's what the GetTransactionStartGeneration() thing above was trying to be a proxy for, but without adding new fields to ReadHandler, to avoid memory bloat. But if we are going to add new fields anyway, why wouldn't we just use those fields to track that information? But ideally we would not add fields.
If we really do want to evict handlers based on when they report, then this PR really needs to explain why that's a useful criterion, in the commit message or code comments (or maybe both).
There was a problem hiding this comment.
Maybe not the right logic.
GetTransactionStartGeneration returns the generation recorded during the creation of the ReadHandler. If the least newly created subscription has the same generation and resource usage as others, it will be evicted when a new subscription is added. This pattern would repeat, with each subsequent new subscription causing the eviction of the least recently created one. As a result, only the least recently created subscription and those created before the handler became full would remain active until the generation updates. Is this the intended behavior?"
Current mechanism of
EnsureResourceForSubscriptionmay evict the Readhandler with more resources or less genaration (older). If same, the latest created one will be selected, in this case, only one subscription can be used for that the new subscription will evict the last one. So add a report timestamp and evict the longest timestamp subscription in this case.