verdier commented on PR #43007:
URL: https://github.com/apache/superset/pull/43007#issuecomment-5250622570

   Semaphore added, as you both asked — and testing it turned up that placement 
matters more than I expected.
   
   My first attempt put admission in `mcp_auth_hook`. It passed at 16 and 20 
concurrent and still exhausted the pool at 40, because `LoggingMiddleware` 
writes the audit record before the hook runs and that write is itself a 
connection checkout (`DBEventLogger failed to log event(s)`). So it is now a 
`ToolConcurrencyMiddleware` sitting outside the logger, and admission encloses 
everything a call does.
   
   On the stock pool, no engine options touched:
   
   | concurrent calls | isolation only | with admission |
   |---|---|---|
   | 16 | 15/16, one `QueuePool` timeout, 35–66s | 16/16 in 4s |
   | 20 | never finished, 5 of 20 rows in 15min | 20/20 in 5s |
   | 40 | — | 40/40 in 160s |
   
   Default limit is `pool_size + max_overflow` read from 
`SQLALCHEMY_ENGINE_OPTIONS` (falling back to SQLAlchemy's 5 + 10), overridable 
via `MCP_MAX_CONCURRENT_TOOL_CALLS`, `0` to disable. Seven tests on the 
limiter, including one that checks a raising call releases its slot — leaking 
admission would wedge the server. Full `tests/unit_tests` green at 11885 passed.
   
   One honest note on what this is: the limit caps the symptom. The tools are 
`async def` while doing blocking DB work, so a call occupies the loop without 
yielding it; running those sections on a worker thread is the fuller answer and 
I have kept it out of this PR deliberately.
   
   @rusackas the comparison with #42629 is in my previous comment — the short 
version is that both approaches measure identically, including the same cliff, 
so the choice is a design one and I have no stake in which lands.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to