jeho-rpls commented on code in PR #16401:
URL: https://github.com/apache/lucene/pull/16401#discussion_r3600077132
##########
lucene/core/src/test/org/apache/lucene/index/TestHnswMergeAbort.java:
##########
@@ -33,52 +36,71 @@
import org.apache.lucene.tests.util.TestUtil;
import org.apache.lucene.util.InfoStream;
import org.apache.lucene.util.NamedThreadFactory;
+import org.apache.lucene.util.ThreadInterruptedException;
/**
- * Tests that aborting a merge (e.g. via {@link IndexWriter#rollback()})
promptly interrupts HNSW
- * graph construction instead of blocking until the entire graph is built.
+ * Tests that aborting a merge (e.g. via {@link IndexWriter#rollback()})
interrupts HNSW graph
+ * construction instead of building the entire graph.
+ *
+ * <p>The merge thread is held at the start of the graph build until the merge
is marked aborted,
+ * and the test then asserts that the build never ran to completion, which
holds at any segment
+ * size. The abort exception is deliberately not asserted on: it is swallowed
as expected control
+ * flow, and a small unchecked build can finish without ever throwing.
*/
public class TestHnswMergeAbort extends LuceneTestCase {
private static final int DIM = 96;
private static final int SEGMENTS = 4;
- private static final int DOCS_PER_SEGMENT = 12_000;
- private static final int BEAM_WIDTH = 250;
+ private static final int DOCS_PER_SEGMENT = 1_000;
+ private static final int BEAM_WIDTH = 100;
+ // always build a graph, no matter how small the segment is
+ private static final int TINY_SEGMENTS_THRESHOLD = 0;
+ private static final int LIVE_DOCS_AFTER_DELETES = SEGMENTS *
DOCS_PER_SEGMENT / 2;
/**
* Every segment carries more than {@code
IncrementalHnswGraphMerger#DELETE_PCT_THRESHOLD}
* 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 {
Review 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.
--
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]
