jeho-rpls commented on PR #16368: URL: https://github.com/apache/lucene/pull/16368#issuecomment-4944211708
Hi @msokolov, thanks for taking a look! > Is this really needed? When do we ever call abortMerges()? It looks to me as if it gets called when an exception happens while closing an IndexWriter, or explicitly rolling back. Seem like kind of exceptional cases `abortMerges()` is routine at the deployment level even though it looks exceptional at the API level. Elasticsearch closes its shard engines through `IndexWriter#rollback` on node shutdown, so every rolling restart of a data node goes through it. That is how we hit this. - When a node is asked to shut down while an HNSW merge is in the middle of graph construction, that rollback blocks until the build completes. In containerized deployments it easily outlives the pod termination grace period, so the process ends up force-killed mid-merge. - In our case, a routine rolling restart of a vector-heavy cluster blocked for 24–31 minutes per node behind a single in-flight merge, past the 30-minute pod grace in the worst case (measurements in #16367). Elasticsearch reports hitting a similar issue in production and has an open downstream workaround (elastic/elasticsearch#152342), but it lives in their stateless plugin only, and the workaround's author notes it is partial coverage. A check in the graph builder covers every directory implementation. > although maybe we would want rollbacks to be faster? Yes, exactly. Faster rollback is the whole motivation. The wait also buys nothing. The writer is rolling back, so the graph being built is discarded the moment it completes. > Do other merge operations check for aborted status? They do. - Writes: every merge output is wrapped via `ConcurrentMergeScheduler#wrapForMerge` into `RateLimitedIndexOutput`, and `MergeRateLimiter#maybePause` throws `MergeAbortedException` once the flag is set. This is the "they periodically check if they are aborted" that `abortMerges` relies on. - Checksums: since #16264 and #16281, every format consumer calls `MergeState#checkAborted` before each producer's integrity check, and `CodecUtil#checksumEntireFile(input, merge)` re-checks the flag every megabyte read. Graph construction is the odd one out. It performs no writes and no checksum reads for tens of minutes, so it never observes the flag. In our profiling it was 93.6% of the merge CPU, while the checksum phase covered by #16281 was 0.1%. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
