Speed up TestHnswMergeAbort and make its assertion deterministic#16401
Conversation
Instead of asserting that rollback() returns within a time limit, hold the merge thread at the start of the graph build until the merge is marked aborted, then assert that the build never ran to completion, observed through InfoStream. This does not depend on segment size or machine speed, so segments shrink from 12k docs to 1k and beam width from 250 to 100. MergingHnswGraphBuilder now emits a completion message like the other two builders, so the graph join path has a completion signal to assert on.
| * deletions, so no source graph is eligible as a base and the merged graph is rebuilt from | ||
| * scratch via {@code HnswGraphBuilder#addVectors}. | ||
| */ | ||
| public void testRollbackDuringFullRebuildMerge() throws Exception { |
There was a problem hiding this comment.
does this test rely on the fact that HNSW merging work is done on a background thread? If we had single-threaded HNSW merging (as we used to do) this would deadlock because the InfoStream would be waiting for the merging operation to be aborted, but it could not progress because it is the merging operation?
I guess that should be OK since all of our mergers do use their own thread pools now
There was a problem hiding this comment.
Thanks for the review!
does this test rely on the fact that HNSW merging work is done on a background thread?
My intent was to depend only on rollback() being called from a different thread than the merge, not on where the merge runs. The test sets that up explicitly, with rollback() on the test thread and forceMerge on its own thread.
If we had single-threaded HNSW merging (as we used to do) this would deadlock because the InfoStream would be waiting for the merging operation to be aborted, but it could not progress because it is the merging operation?
I think that cycle is avoided because the release signal does not come from the merge side. It comes from the rollback thread, which marks every running merge aborted inside IndexWriter#abortMerges and emits the now wait for message before it ever starts waiting. The blocking branch in the test only fires for the HNSW build graph message, so the rollback thread is never held there. As a backstop the latch has a 2 minute timeout, so a missed release fails the releasedAfterAbort assertion instead of hanging the build.
I guess that should be OK since all of our mergers do use their own thread pools now
It holds even without that. I swapped ConcurrentMergeScheduler for SerialMergeScheduler locally, so the merge runs synchronously inside forceMerge, and all three tests still pass across repeated runs.
There was a problem hiding this comment.
thanks, that makes sense. I'll merge today
Fixes #16397.
The tests asserted that
rollback()returns within 10 seconds, which needed big segments (4 x 12k docs, beam width 250) to stay reliable, about 50s per test.Changes:
InfoStreamthat the graph build never ran to completion. To make that deterministic at any size, the testInfoStreamholds the merge thread at thebuild graphmessage untilIndexWriter#abortMergeshas marked the merge aborted, signaled by thenow wait for N running merge/s to abortmessage.MergingHnswGraphBuildernow emits a completion message like the other two builders (addVectors [...),merge completed: ...), so the graph join path has a completion signal to assert on.build graphstart message to pin its intended merge path (full rebuild, graph join, concurrent with 2 workers).tinySegmentsThreshold=0so a graph is always built.Testing:
-Dtests.iters=20: 60/60 pass.TestHnswMergeAbortslows down build #16397).