magibney commented on code in PR #2313:
URL: https://github.com/apache/solr/pull/2313#discussion_r1574778689
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1048,7 +1056,7 @@ private static class AsyncTracker {
int getMaxRequestsQueuedPerDestination() {
Review Comment:
Yes. again following on my earlier dive into this, my conclusion was that
`maxRequestsQueuedPerDestination` is close to irrelevant as a user-configurable
value. This is counterintuitive because when we actually get an exception, the
exception is because this queue has overflowed. But there's no actual
throttling behavior implemented by this queue, and it really makes more sense
to think of the per-host setting as a buffer, to _very_ temporarily hold
requests in between them being submitted for sending, and them actually being
sent.
It's illustrative to consider that whether or not these two values are
independently configurable, you could max out the configured global outstanding
request limit with requests to a _single_ host, which would cause the
`maxRequestsQueuedPerDestination` queues to overflow for _all_ hosts, throwing
exceptions, but without any requests actually having been sent to any host
other than the host the single host that consumed all the configured global
semaphore permits. IMO in this context a configurable per destination "limit"
is not really useful for anything, and is certainly confusing.
The [approach I'd
recommend](https://github.com/cowpaths/fullstory-solr/pull/124#discussion_r1294915329)
is to have the per-destination limit be hardcoded (it's useless at best,
misleading at worst), and clarify that the configurable value affects
connection throttling over _all_ hosts:
>I think the right way to think about maxRequestsQueuedPerDestination is:
How many requests (per destination) is it reasonable to buffer, figuring "I'll
send this as soon as I can" vs failing fast and sending an error code to the
client indicating that we're too backed up right now. I could even see just
making this a static value (e.g., the 3k that it has effectively been up to
this point). I think I was thrown by the "comfortably above max outstanding
requests" comment though, implying some relationship there. Tbh I can't think
of any reason why this should even necessarily be "above" max outstanding
requests, let alone "comfortably above" ...
--
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]
