Disable concurrent search for filter duplicates in significant_text#20857

Merged

andrross merged 1 commit into

opensearch-project:mainfrom

andrross:DuplicateByteSequenceSpotter-concurrent-search

Mar 17, 2026

Member

andrross commented Mar 13, 2026

The underlying structured used to find duplicate byte sequences (DuplicateByteSequenceSpotter) is stateful and not thread-safe, so disable concurrent search if this particular feature is being used.

Related Issues

Part of #14408, specifically {yaml=search.aggregation/90_sig_text/Dedup noise}

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.

andrross requested review from a team, Bukhtawar, CEHENKLE, Rishikesh1159, anasalkouz, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners

March 13, 2026 02:40

Contributor

github-actions Bot commented Mar 13, 2026 •

edited

Loading

PR Reviewer Guide 🔍

(Review updated until commit `400dcbb`)

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 Thread Safety The fix disables concurrent search when `filterDuplicateText` is true, but it's worth validating that `filterDuplicateText` is correctly set and that there are no other shared stateful structures in the aggregator that could also be non-thread-safe when concurrent search is enabled (i.e., when `filterDuplicateText` is false). return filterDuplicateText == false;

andrross force-pushed the DuplicateByteSequenceSpotter-concurrent-search branch from 39b62ab to cca1a3b Compare

March 13, 2026 02:41

Contributor

github-actions Bot commented Mar 13, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use idiomatic boolean negation operator Using `== false` for boolean negation is unconventional in Java. The idiomatic and more readable way to negate a boolean is to use the `!` operator. This improves code clarity and follows standard Java conventions. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java [321] -return filterDuplicateText == false; +return !filterDuplicateText; Suggestion importance[1-10]: 4 __ Why: The suggestion is valid - using `!filterDuplicateText` is more idiomatic Java than `filterDuplicateText == false`. However, this is a minor style improvement with no functional impact.	Low

Member Author

andrross commented Mar 13, 2026

@msfroh If you don't mind I'd appreciate a look to see if this seems correct

Contributor

github-actions Bot commented Mar 13, 2026

Persistent review updated to latest commit cca1a3b

Contributor

github-actions Bot commented Mar 13, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

opensearch-ci-bot mentioned this pull request

[AUTOCUT] Gradle Check Flaky Test Report for ShardsLimitAllocationDeciderIT #19726

Closed

Contributor

github-actions Bot commented Mar 13, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

This was referenced Mar 13, 2026

[AUTOCUT] Gradle Check Flaky Test Report for IndexShardTests #14321

Open

[AUTOCUT] Gradle Check Flaky Test Report for TelemetryMetricsEnabledSanityIT #19422

Open

Contributor

github-actions Bot commented Mar 13, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Contributor

github-actions Bot commented Mar 14, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

mch2 approved these changes

View reviewed changes

This was referenced Mar 14, 2026

[AUTOCUT] Gradle Check Flaky Test Report for FullRollingRestartIT #18490

Open

[AUTOCUT] Gradle Check Flaky Test Report for Netty4Http3IT #20654

Closed

Contributor

github-actions Bot commented Mar 14, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Contributor

github-actions Bot commented Mar 14, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

opensearch-ci-bot mentioned this pull request

[AUTOCUT] Gradle Check Flaky Test Report for AutoForceMergeManagerIT #20656

Open

Contributor

github-actions Bot commented Mar 17, 2026

❌ Gradle check result for cca1a3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?


          Disable concurrent search for filter duplicates in significant_text

400dcbb

The underlying structured used to find duplicate byte sequences
(DuplicateByteSequenceSpotter) is stateful and not thread-safe, so
disable concurrent search if this particular feature is being used.

Signed-off-by: Andrew Ross <andrross@amazon.com>

andrross force-pushed the DuplicateByteSequenceSpotter-concurrent-search branch from cca1a3b to 400dcbb Compare

March 17, 2026 19:19

Contributor

github-actions Bot commented Mar 17, 2026

Persistent review updated to latest commit 400dcbb

Contributor

github-actions Bot commented Mar 17, 2026

❕ Gradle check result for 400dcbb: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

prudhvigodithi approved these changes

View reviewed changes

Member

prudhvigodithi commented Mar 17, 2026

We can explore supporting DuplicateByteSequenceSpotter thread safe as an enhancement to ensure significant_text aggs with filter_duplicate_text get benefit with concurrent segment search.

andrross merged commit f46ce23 into opensearch-project:main

33 checks passed

andrross deleted the DuplicateByteSequenceSpotter-concurrent-search branch

March 17, 2026 21:08

opensearch-ci-bot mentioned this pull request

[AUTOCUT] Gradle Check Flaky Test Report for AzureBlobStoreRepositoryTests #14291

Open

codecov Bot commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.28%. Comparing base (28fa177) to head (400dcbb).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...bucket/terms/SignificantTextAggregatorFactory.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20857      +/-   ##
============================================
- Coverage     73.32%   73.28%   -0.05%     
- Complexity    72272    72318      +46     
============================================
  Files          5797     5802       +5     
  Lines        330323   330405      +82     
  Branches      47676    47686      +10     
============================================
- Hits         242215   242123      -92     
- Misses        68663    68903     +240     
+ Partials      19445    19379      -66

☔ 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.

sohami reviewed

View reviewed changes

...n/java/org/opensearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java

                   @Override
                   protected boolean supportsConcurrentSegmentSearch() {
-                      return true;
+                      // The underlying structured used to find duplicate byte sequences (DuplicateByteSequenceSpotter)

Contributor

sohami Mar 18, 2026

it is unclear why DuplicateByteSequenceSpotter needs to be threadsafe for inter-segment concurrent search. I see it being created at per leaf (or segment) and owning ord combination here

github-actions Bot mentioned this pull request

Use caching to avoid excessive creation of NonClosingReaderWrapper instances #20921

Merged

3 tasks

andrross mentioned this pull request

[AUTO] Add release notes for 3.6.0 #21045

Merged

opensearch-ci-bot mentioned this pull request

[AUTO] Add release notes for 3.6.0 Draft 2 #21115

Merged

aparajita31pandey pushed a commit to aparajita31pandey/OpenSearch that referenced this pull request


          Disable concurrent search for filter duplicates in significant_text (o…

85b9261

…pensearch-project#20857)

The underlying structured used to find duplicate byte sequences
(DuplicateByteSequenceSpotter) is stateful and not thread-safe, so
disable concurrent search if this particular feature is being used.

Signed-off-by: Andrew Ross <andrross@amazon.com>
Signed-off-by: Aparajita Pandey <aparajita31pandey@gmail.com>

pradeep-L pushed a commit to pradeep-L/OpenSearch that referenced this pull request


          Disable concurrent search for filter duplicates in significant_text (o…

ae0c4ae

…pensearch-project#20857)

The underlying structured used to find duplicate byte sequences
(DuplicateByteSequenceSpotter) is stateful and not thread-safe, so
disable concurrent search if this particular feature is being used.

Signed-off-by: Andrew Ross <andrross@amazon.com>

This was referenced Apr 27, 2026

[AUTOCUT] Gradle Check Flaky Test Report for AzureBlobStoreRepositoryTests #21372

Closed

[AUTOCUT] Gradle Check Flaky Test Report for FullRollingRestartIT #21376

Closed

[AUTOCUT] Gradle Check Flaky Test Report for TelemetryMetricsEnabledSanityIT #21389

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sohami sohami left review comments

mch2 mch2 approved these changes

prudhvigodithi prudhvigodithi approved these changes

anasalkouz Awaiting requested review from anasalkouz

ashking94 Awaiting requested review from ashking94 ashking94 is a code owner

Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

CEHENKLE Awaiting requested review from CEHENKLE

cwperks Awaiting requested review from cwperks cwperks is a code owner

dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

gbbafna Awaiting requested review from gbbafna gbbafna is a code owner

jed326 Awaiting requested review from jed326 jed326 is a code owner

kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

msfroh Awaiting requested review from msfroh msfroh is a code owner

owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

reta Awaiting requested review from reta reta is a code owner

Rishikesh1159 Awaiting requested review from Rishikesh1159

sachinpkale Awaiting requested review from sachinpkale sachinpkale is a code owner

saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

shwetathareja Awaiting requested review from shwetathareja shwetathareja is a code owner

Labels

None yet