Search might not return on thread pool rejection #6032
Conversation
When a thread pool rejects the execution on the local node, the search might not return. This happens due to the fact that we move to the next shard only *within* the execution on the thread pool in the start method. If it fails to submit the task to the thread pool, it will go through the fail shard logic, but without "counting" the current shard itself. When this happens, the relevant shard will then execute more times than intended, causing the total opes counter to skew, and for example, if on another shard the search is successful, the total ops will be incremented *beyond* the expectedTotalOps, causing the check on == as the exit condition to never happen. The fix here makes sure that the shard iterator properly progresses even in the case of rejections, and also includes improvement to when cleaning a context is sent in case of failures (which were exposed by the test). Though the change fixes the problem, we should work on simplifying the code path considerably, the first suggestion as a followup is to remove the support for operation threading (also in broadcast), and move the local optimization execution to SearchService, this will simplify the code in different search action considerably, and will allow to remove the problematic #firstOrNull method on the shard iterator. The second suggestion is to move the optimization of local execution to the TransportService, so all actions will not have to explicitly do the mentioned optimization. fixes elastic#4887
|
@clintongormley doesn't look like it at first glance... |
| logger.trace("failed to release context", t1); | ||
| } | ||
| } | ||
| listener.onFailure(t); |
There was a problem hiding this comment.
maybe put the for loop and onFailure call into a try final block?
There was a problem hiding this comment.
I didn't see a reason where this will require a try .., finally, since the calls to release the context are properly protected. I think its enough?
|
Sneaky bug! Left one minor comment, but other than that LGTM. |
|
In which version is this fixed? I've got a 1.1.2 and a single node.js process with async queries is running into the issue. Could you pls. recommend me quickly an alternative (1.2 got again breaking changes ...) |
|
this is fixed in 1.1.2, are you sure you are running into this problem and not something else maybe? Can you open a new issue so we can track it, with a recreation if you can which would help tremendously |
clintongormley
added
the
:Search/Search
When a thread pool rejects the execution on the local node, the search might not return.
This happens due to the fact that we move to the next shard only within the execution on the thread pool in the start method. If it fails to submit the task to the thread pool, it will go through the fail shard logic, but without "counting" the current shard itself. When this happens, the relevant shard will then execute more times than intended, causing the total opes counter to skew, and for example, if on another shard the search is successful, the total ops will be incremented beyond the expectedTotalOps, causing the check on == as the exit condition to never happen.
The fix here makes sure that the shard iterator properly progresses even in the case of rejections, and also includes improvement to when cleaning a context is sent in case of failures (which were exposed by the test).
Though the change fixes the problem, we should work on simplifying the code path considerably, the first suggestion as a followup is to remove the support for operation threading (also in broadcast), and move the local optimization execution to SearchService, this will simplify the code in different search action considerably, and will allow to remove the problematic #firstOrNull method on the shard iterator.
The second suggestion is to move the optimization of local execution to the TransportService, so all actions will not have to explicitly do the mentioned optimization.
fixes #4887