Abort stateless merge reads on shard close to unblock HNSW merges - #152342
Abort stateless merge reads on shard close to unblock HNSW merges#152342eranweiss-elastic wants to merge 22 commits into
Conversation
|
Hi @eranweiss-elastic, 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?
|
| } | ||
|
|
||
| private void readInternalSlow(ByteBuffer b, long position, int length) throws Exception { | ||
| cacheFileReader.read( |
There was a problem hiding this comment.
these calls are the expensive ones (cache miss). perhaps we should check in readInternalSlow as well?
There was a problem hiding this comment.
Good idea, I'll add. Thanks.
|
Can someone from @elastic/es-distributed have a look as well please as this is more merge/caching related ? |
|
We saw this issue in prod yesterday leading to red health (see the linked internal ticket). Closest issue description for the problem seems to be apache/lucene#16367. I was wondering if this PR handles this issue (for stateless) as described in that Lucene issue or it is different? |
|
@pxsalehi I this covers some of that, but it's not a 100% coverage. I think it could be extended to handle it |
|
thanks, @eranweiss-elastic. In that case, I'm wondering if we should close /incident-management/issues/1037 with this ticket. If this is still going to happen sometimes then we might want a ticket to refer to in incidents so it is clear what is done and what is left. Could we instead add more context to /incident-management/issues/1037 and mention what's fixed and what's left? Or alternatively could you open a ticket documenting the case that is left? |
851f12c to
4f728a2
Compare
|
@pxsalehi this should resolve /incident-management/issues/1037, it's just not exactly the same as apache/lucene#16367 |
tlrx
left a comment
There was a problem hiding this comment.
I don't think this will work in most cases, because it assumes that index inputs are opened and read with a specific IO context, which I think is never the case for KNN vectors readers in Lucene.
I think the right way to address this issue is in Lucene like apache/lucene#16368 does. I'm not sure what this change adds that apache/lucene#16368 don't do, but if something is missing we should try to implement it into Lucene. The current change as proposed introduces non trivial complexity.
| } | ||
|
|
||
| private void checkMergeReadAborted(IOContext context) throws IOException { | ||
| if (context.context() == IOContext.Context.MERGE) { |
There was a problem hiding this comment.
This assumes that IndexInput instances used in merges are opened with a special IOContext.Context.MERGE on indexing nodes. I think that most of the time Lucene will reuse already opened readers for merging (NRT readers from the IndexWriter pool), so this condition will rarely be met.
|
@tlrx I wasn't aware that apache/lucene#16368 was merged. I agree that it's close enough to probably not be worth the added complexity. I'm going to pause work on this, and see if waiting for the Lucene update is good enough product-wise. |
|
@tlrx given that this issue keeps tripping customers, I want to fix it before the next lucene release. I'm thinking about using this PR (after fixing the problems in it) as a temporary fix, until the lucene release. What do you think? |
Fix slow stateless shard close when BBQ/HNSW merges are in progress by cooperatively aborting merge reads on blob-cache inputs.
Signal abort from
IndexEngine.close()and merge-schedulerclose()beforeIndexWriter.rollback(). Merge-context reads onBlobCacheIndexInputandReopeningIndexInputthrowMergeAbortedExceptiononce the shard is closing.Closes: https://github.com/elastic/incident-management/issues/1037