Repository navigation
Fix leaking bulk task on shard dispatch failure - #158112
Conversation
|
Pinging @elastic/es-distributed (Team:Distributed) |
🔍 Preview links for changed docs⏳ Building and deploying preview... View progress This comment will be updated with preview links when the build is complete. |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
DaveCTurner
left a comment
There was a problem hiding this comment.
I think I'd rather fix this with client.execute(), see comment.
| try { | ||
| client.executeLocally(TransportShardBulkAction.TYPE, bulkShardRequest, shardListener); |
There was a problem hiding this comment.
Could we instead call client.execute()? That delegates into client.executeLocally() but will feed exceptions back to the listener as we want; it accepts a TaskCancelledException ok (see org.elasticsearch.client.internal.node.NodeClient#doExecute) while failing tests on other exceptions deemed to be unacceptable (see org.elasticsearch.client.internal.support.AbstractClient#execute).
|
FWIW I've opened #158264 to address this more generally |
💔 Backport failedThe backport operation could not be completed due to the following error: You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
implicitly fixed by elastic#158112 Closes elastic#156929 Closes elastic#152925 Closes elastic#152291 Closes elastic#157763
fix #158019
NodeClient#executeLocallythrows instead of notifying the listener when it cannot start a request at all: a cancelled parent task, oversized task headers, or an unresolvable action.BulkOperation#executeBulkShardRequestcalled it without a catch, so the ref acquired frombulkItemRequestCompleteRefCountfor that shard request was never released and the bulk operation never completed. On the failure store redirect path the throw lands in aRefCountingRunnabledelegate, which logsexception in delegateand swallows it, leaving the bulk task registered forever.This PR completes the shard listener with the exception instead, which records the shard level failure, fails the shard's items and releases the ref.
TransportAction#executeis exception-safe, so every wayexecuteLocallycan throw happens before the action runs and the listener is still uncompleted at that point.