Handle rejection in PrioritizedThrottledTaskRunner

[DaveCTurner]

Today PrioritizedThrottledTaskRunner submits a bare Runnable to the executor, wrapping around the Runnable received from the caller, which effectively assumes that tasks are never rejected from the threadpool. However this utility accepts any Executor, so this is not a safe assumption to make. This commit moves to submitting an AbstractRunnable to the executor, and to requiring callers to pass in an AbstractRunnable, so that failures and rejections can be handled properly.

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[elasticsearchmachine]

Hi @DaveCTurner, I've created a changelog YAML for you.

[DaveCTurner]

>non-issue because the places this is used today don't ever reject tasks.

[idegtiarenko]

Should we assert at least one rejection happened?

[DaveCTurner]

Sure, I think we can do that. See 0fc9484.

[pxsalehi]

I was wondering since we now use AbstractRunnable, would it make sense to also require an EsThreadPoolExecutor instead of an Executor? From what I understand, isForceExecution is enforced only if we use one of our own Executors.

[DaveCTurner]

I think not, for instance because ThreadPool#executor doesn't return an EsThreadPoolExecutor so we'd have to do some casting.

IMO it's up to the caller to use an executor which is appropriate for their tasks. If they're relying on isForceExecution working then they'd better pass in an executor that uses it.

[DaveCTurner]

@elasticmachine please run elasticsearch-ci/part-2

[DaveCTurner]

I could see some value in accepting a plain Runnable and then checking if it's an AbstractRunnable within the runner, much like we do with EsThreadPoolExecutor, but I do prefer this approach. If anything I'd rather make EsThreadPoolExecutor refuse to work with plain old Runnable things. It's all too easy to miss stuff like rejection handling today, leading to things like #92449. But that's a whole other discussion...