Fix flaky SegmentReplicationIT remote build test - #3550
Conversation
Signed-off-by: Navaneeth Rao <navaneethbv@users.noreply.github.com>
PR Reviewer Guide 🔍(Review updated until commit fd8fd4b)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit fd8fd4b |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3550 +/- ##
=========================================
Coverage 83.20% 83.20%
Complexity 4631 4631
=========================================
Files 469 469
Lines 16572 16572
Branches 2181 2181
=========================================
Hits 13788 13788
Misses 1959 1959
Partials 825 825 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| assertBusy(() -> { | ||
| Response replicaSearchResponse = performSearch(INDEX_NAME, queryBuilder.toString(), "preference=_replica"); | ||
| String replicaResponseBody = EntityUtils.toString(replicaSearchResponse.getEntity()); | ||
| List<KNNResult> replicaResults = parseSearchResponse(replicaResponseBody, FIELD_NAME); | ||
| assertEquals(docsInIndex - deleteDocs, replicaResults.size()); | ||
| }); |
There was a problem hiding this comment.
what will happen in the case when segment replication is really struck due to bugs? how we will get out of the busy loop here?
There was a problem hiding this comment.
assertBusy is bounded and won't retry indefinitely. The default implementation retries for up to 10 seconds. If segment replication is genuinely stuck and the replica never reaches the expected state, the assertion will continue failing, and assertBusy will eventually propagate the failure, causing the test to fail. The intent here is only to tolerate the asynchronous replication delay instead of relying on the fixed 5-second sleep.
There was a problem hiding this comment.
The solution provided in the pR: #3397 was better than this, since it avoids the fixed delay.
2 participants
Description
Re-enable
SegmentReplicationITin the remote index build integration test suite now that remote builds terminate when their Lucene merge is aborted by #3488.Remove the fixed five-second sleep from
testSearchOnReplicas_whenIndexHasDeletedDocs_thenSuccess.The primary KNN assertion remains immediate so the test still detects an invalid remote graph instead of waiting for a later merge to hide it.
Only the replica assertion uses
assertBusy, because segment replication is asynchronous and the assertion should wait for the actual observable condition rather than an arbitrary delay.This is a smaller alternative to #3397 and, unlike that change, removes the Gradle exclusion so the remote index build workflow exercises the regression.
Local validation:
./gradlew spotlessCheck --no-build-cache./gradlew compileTestJava -x cmakeJniLib -x buildJniLib --no-build-cacheJAVA_HOME="/Library/Java/JavaVirtualMachines/temurin-21.jdk/Contents/Home" CPPFLAGS="-I/opt/homebrew/opt/libomp/include" LDFLAGS="-L/opt/homebrew/opt/libomp/lib" ./gradlew :integTest --tests 'org.opensearch.knn.index.SegmentReplicationIT.testSearchOnReplicas_whenIndexHasDeletedDocs_thenSuccess' -PnumNodes=2 --no-build-cacheThe focused two-node integration test passed locally in 4 minutes 40 seconds after installing Homebrew GCC and libomp.
The remote index build variant additionally requires the containerized remote builder and LocalStack, so the repository's remote index build workflow remains the end-to-end validation gate for the reported configuration.
Related Issues
Resolves #2726
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.