Expand some threadpool docs #123244
Expand some threadpool docs #123244
Conversation
DaveCTurner
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
DiannaHohensee
left a comment
DiannaHohensee
left a comment
There was a problem hiding this comment.
This improvement lead me to a lot of further questions 😅
server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Outdated
Show resolved
Hide resolved
DiannaHohensee
left a comment
DiannaHohensee
left a comment
There was a problem hiding this comment.
LGTM. Some comments remain, but they're minor. Looks nice.
| * performance (especially if using spinning disks). | ||
| * <p> | ||
| * Blocking on a future on this pool risks deadlock if there's a chance that the completion of the future depends on work being done | ||
| * on this pool. Unfortunately that's pretty likely in most cases because of how often this pool is used; it's really rare because |
There was a problem hiding this comment.
This made me wonder about the details of how a deadlock could occur, so that one can avoid it properly. Seemed to suggest Executor tasks are executed in order, of submission perhaps, so I was thinking maybe a task waiting for the future is put on the queue first. But that would mean a caller queues the future task before the task that it waits on. Is that really the scenario, or is there something else at play?
In short, if you could elaborate on the particulars of how the deadlock can happen, that would help readers not to do it.
There was a problem hiding this comment.
I don't think execution order matters (although yes the queue is FIFO). It's just what it says: blocking generic threads on futures risks deadlock if the completion of those futures requires work to be done on the generic pool. There's no way to do that work if all the threads that could do it are blocked.
There was a problem hiding this comment.
Thanks for referencing UnsafePlainActionFuture 👍
| * on this pool. Unfortunately that's pretty likely in most cases because of how often this pool is used; it's really rare because | ||
| * of the high limit on the pool size, but when it happens it is extremely harmful to the node. | ||
| * <p> | ||
| * This pool is for instance used for recovery-related work, which is a mix of CPU-bound and IO-bound work. The recovery subsystem |
There was a problem hiding this comment.
I'd bump this paragraph higher, swap it with the above deadlock paragraph. It looks like a natural continuation of the CPU/IO paragraphs discussions, and deadlocking doesn't participate in that line of reasoning (but currently interrupts it).
There was a problem hiding this comment.
I left it where it is but clarified that recovery also doesn't block on futures, so this now follows more logically from the preceding paragraphs.
DaveCTurner
added
auto-merge-without-approval
3 participants
No description provided.