Skip to content

Fix flaky DerivedSourceLeafReaderTests#20904

Merged
mgodwan merged 1 commit intoopensearch-project:mainfrom
andrross:flaky-DerivedSourceLeafReaderTests
Mar 18, 2026
Merged

Fix flaky DerivedSourceLeafReaderTests#20904
mgodwan merged 1 commit intoopensearch-project:mainfrom
andrross:flaky-DerivedSourceLeafReaderTests

Conversation

@andrross
Copy link
Member

testWithRandomDocuments was flaky due to two issues:

  1. NoMergePolicy blocked forceMerge(1), so randomized IndexWriterConfig settings that caused multiple flushes resulted in multiple segments instead of the expected one.

  2. The test stored _source in the index, but DerivedSourceStoredFields injects _source via the source provider then delegates to the underlying stored fields. This caused the visitor to receive _source twice, with the second call using Lucene's internal buffer which could differ from the original bytes.

Fix by removing NoMergePolicy so forceMerge(1) works, and not storing _source in the index, which matches the production use case where derived source synthesizes _source from other data.

Related Issues

Resolves #20812

Check List

  • Functionality includes testing.

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.

testWithRandomDocuments was flaky due to two issues:

1. NoMergePolicy blocked forceMerge(1), so randomized
   IndexWriterConfig settings that caused multiple flushes
   resulted in multiple segments instead of the expected one.

2. The test stored _source in the index, but
   DerivedSourceStoredFields injects _source via the source
   provider then delegates to the underlying stored fields.
   This caused the visitor to receive _source twice, with the
   second call using Lucene's internal buffer which could
   differ from the original bytes.

Fix by removing NoMergePolicy so forceMerge(1) works, and not
storing _source in the index, which matches the production use
case where derived source synthesizes _source from other data.

Signed-off-by: Andrew Ross <andrross@amazon.com>
@andrross andrross requested a review from a team as a code owner March 17, 2026 23:46
@github-actions github-actions bot added >test-failure Test failure from CI, local build, etc. autocut flaky-test Random test failure that succeeds on second run labels Mar 17, 2026
@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Unused Variable

The source byte array is still generated via randomByteArrayOfLength and stored in docIdToSource, but it is no longer added to the document as a stored field. The map is presumably used later to verify that the derived source provider returns the correct bytes. It should be verified that the test still correctly validates the source content, since the document no longer stores _source and the map values are random bytes unrelated to any stored field.

byte[] source = randomByteArrayOfLength(randomIntBetween(10, 50));
docIdToSource.put(i, source);
Document doc = new Document();
doc.add(new StoredField("field", i));
randomWriter.addDocument(doc);

@github-actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore correct stored field for source verification

The original test stored _source bytes and verified them via docIdToSource, but now
the field name is changed to "field" with an integer value. This likely breaks the
test's ability to verify source data retrieval through DerivedSourceLeafReader,
since the map docIdToSource still maps doc IDs to byte arrays but the stored field
no longer contains those bytes. The field name and type should remain consistent
with what DerivedSourceLeafReader expects to read.

server/src/test/java/org/opensearch/common/lucene/index/DerivedSourceLeafReaderTests.java [159-160]

-doc.add(new StoredField("field", i));
+doc.add(new StoredField("_source", source));
 randomWriter.addDocument(doc);
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about changing the stored field from "_source" with byte array to "field" with integer value, which could affect the test's ability to verify source data retrieval. However, this appears to be an intentional change in the PR (possibly to test a different aspect of DerivedSourceLeafReader), and without seeing the full test context (especially how docIdToSource is used later), it's unclear if this is actually a bug.

Low

@github-actions
Copy link
Contributor

✅ Gradle check result for 4766166: SUCCESS

@codecov
Copy link

codecov bot commented Mar 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.39%. Comparing base (28fa177) to head (4766166).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #20904      +/-   ##
============================================
+ Coverage     73.32%   73.39%   +0.06%     
- Complexity    72272    72387     +115     
============================================
  Files          5797     5802       +5     
  Lines        330323   330405      +82     
  Branches      47676    47686      +10     
============================================
+ Hits         242215   242497     +282     
+ Misses        68663    68514     -149     
+ Partials      19445    19394      -51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mgodwan mgodwan merged commit 3548ff9 into opensearch-project:main Mar 18, 2026
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autocut flaky-test Random test failure that succeeds on second run lucene skip-changelog >test-failure Test failure from CI, local build, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[AUTOCUT] Gradle Check Flaky Test Report for DerivedSourceLeafReaderTests

2 participants