new(libsinsp): async event queue capacity setter #1815
Conversation
There was a problem hiding this comment.
Hi @mrgian! Thanks for your PR.
Personally, I wouldn't mess with defaults. I'd recommend creating a setter for it since this will likely have a performance impact on Falco and affect more than just your use case.
If other maintainers decide that it is OK to increase the default size for the async queue, I would still like to get a setter implemented, so that adopters can roll it back at runtime.
On the other hand, if a setter is not possible to implement, turning this parameter into a compile time value could also work for adopters, but a runtime configuration setter might be more flexible.
|
Hi @Molter73 |
|
/milestone 0.17.0 |
mrgian
force-pushed
the
increase-async-event-queue
branch
from
b8cd300 to
a0c516d
Compare
mrgian
changed the title
| std::scoped_lock<Mtx> lk(m_mtx); | ||
| if(m_queue.size() <= capacity) | ||
| { | ||
| m_capacity = capacity; |
There was a problem hiding this comment.
Am I missing something here or why do we not have a call m_queue.set_capacity to actually change the capacity? Or is it set later? In that case I would find the method name set_capacity confusing.
There was a problem hiding this comment.
The capacity here is just artificially set by us to bound the queue's size -- we don't really care about what the underlying priority queue's capacity actually is, and I don't even think we can/should interfere with that
There was a problem hiding this comment.
Oh interesting, hmmm isn't the standard approach to set and bound the capacity of the actual queue at init time and let the queue handle scenarios if its full. That's basically what we now do for the Falco outputs queue. WDYT?
There was a problem hiding this comment.
Yeah I agree -- the only problem is that the queue is owned by the inspector currently, so in order for the value to be expressed at construction time we'd need a mechanism for either passing the value at the inspector init-time (a bit overkill) or for swapping the queue with another one. If we get to that, I'd rather prefer having a queue that allows resizing at runtime. It's not much cost in terms of synchronization, plus in the real world we'll always resize it when empty anyways (e.g. before opening the inspector).
There was a problem hiding this comment.
I'd rather prefer having a queue that allows resizing at runtime
If that's a requirement, perhaps leave a note in the code base.
Signed-off-by: Gianmatteo Palmieri <[email protected]>
mrgian
force-pushed
the
increase-async-event-queue
branch
from
a0c516d to
866337e
Compare
|
LGTM label has been added. Git tree hash: 38a9e8b0ab8c6d67ed7883cf6db41ec09fb50f7c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: incertum, jasondellaluce, mrgian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
Any specific area of the project related to this PR?
Does this PR require a change in the driver versions?
What this PR does / why we need it:
We are using this queue more and more, especially by plugins sending asynchronous events.
For this reason the hardcoded maximum queue size of 1000 elements is becoming restrictive.
This PR increases the async event queue maximum size to 30000.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: