Addressing inference timeout issues on inference operator tests#154972
Addressing inference timeout issues on inference operator tests#154972pmpailis wants to merge 4 commits into
Conversation
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
|
Hi @pmpailis, I've created a changelog YAML for you. |
🔍 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?
|
…es_on_inference_operator_tests
| if (requestItemIterator.hasNext() == false) { | ||
| // Re-check under the lock: completeIfFinished() releases the request iterator (and its backing block) while holding | ||
| // this lock when a failure occurs. Without re-checking, a concurrent poller that passed the guard above could invoke | ||
| // requestItemIterator.next() on an already-released block. See https://github.com/elastic/elasticsearch/issues/154866 |
There was a problem hiding this comment.
shoudl we remove mention of the issue here: "#154866" and keep comment general overall.
mayya-sharipova
left a comment
There was a problem hiding this comment.
The fix LGTM, but I am not an expert in this area!
There seems be a race condition for when an inference request fails. Assume the following scenario:
When a bulk inference operation fails,
completeIfFinished()invokes the completion listener under thecheckpointlock, which releases the request iterator and its backing input block (viaActionListener.releaseBefore(requests, listener)inperformAsync). Then, the driver thread keeps polling the re-queued operation viapollNextRequest(), which checkshasFailure() || completed.get()before acquiring thecheckpointlock, so the check could be stale:E.g.:
synchronized (checkpoint).requestItemIterator.next()on the released block.Reading the released block trips a
MockBigArraysassertion on the driver thread. Since that's anAssertionError(not caught byperformAsync'scatch (Exception e)), it kills the driver and the operation never completes, which the test observes as a timeout.So in this PR, we simply reuse the check outside the lock, to ensure that the operation is still valid.
Closes #154866