Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unmute nested sort related ITs #11377

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Conversation

jed326
Copy link
Collaborator

@jed326 jed326 commented Nov 28, 2023

Description

Unmuting nested sort related tests for concurrent segment search. It's not super obvious why but both these tests and the underlying race condition itself was addressed by #11249.

The race condition comes from when SortBuilder::buildSort is called simultaneously by multiple threads which results in the LinkedListed in NestedScope being accessed concurrently leading to the stack traces found in #11187 & #11258.

In #11249 this was changed here to instead pass in the same SortAndFormats from the search context instead of rebuilding it on each thread. This means that we won't run into this same race condition anymore for nested sorts and I also verified that SortBuilder::buildSort is not called in a multi-threaded fashion from anywhere else.

Thus since the underlying problem has been addressed, unmuting these tests.

Related Issues

Resolves #11187
Resolves #11258

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@github-actions github-actions bot added bug Something isn't working flaky-test Random test failure that succeeds on second run labels Nov 28, 2023
@jed326
Copy link
Collaborator Author

jed326 commented Nov 28, 2023

@reta @sohami this PR is just unmuting some tests but please let me know if you would like to discuss anything else related to the original issues. Thanks!

Copy link
Contributor

❌ Gradle check result for 7d7d798: null

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?

Copy link
Contributor

github-actions bot commented Nov 28, 2023

Compatibility status:

Checks if related components are compatible with change 919c33d

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/performance-analyzer.git]

Signed-off-by: Jay Deng <jayd0104@gmail.com>
@jed326 jed326 added the backport 2.x Backport to 2.x branch label Nov 29, 2023
Copy link
Contributor

❌ Gradle check result for 919c33d: 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?

@sohami
Copy link
Collaborator

sohami commented Nov 29, 2023

@reta @sohami this PR is just unmuting some tests but please let me know if you would like to discuss anything else related to the original issues. Thanks!

I guess you can update the Description as root cause is now known.

It's not super obvious why but both these tests and the underlying race condition itself was addressed by https://github.com/opensearch-project/OpenSearch/pull/11249.

Copy link
Contributor

❕ Gradle check result for 919c33d: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testIndexReopenClose

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

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ec5a0f9) 71.37% compared to head (919c33d) 71.31%.
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #11377      +/-   ##
============================================
- Coverage     71.37%   71.31%   -0.06%     
- Complexity    58964    58966       +2     
============================================
  Files          4890     4890              
  Lines        277468   277468              
  Branches      40313    40313              
============================================
- Hits         198029   197873     -156     
- Misses        62945    63125     +180     
+ Partials      16494    16470      -24     

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

@reta reta merged commit 530a93b into opensearch-project:main Nov 29, 2023
32 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 29, 2023
Signed-off-by: Jay Deng <jayd0104@gmail.com>
(cherry picked from commit 530a93b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Nov 29, 2023
(cherry picked from commit 530a93b)

Signed-off-by: Jay Deng <jayd0104@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
fahadshamiinsta pushed a commit to fahadshamiinsta/OpenSearch270 that referenced this pull request Dec 4, 2023
Signed-off-by: Jay Deng <jayd0104@gmail.com>
deshsidd pushed a commit to deshsidd/OpenSearch that referenced this pull request Dec 11, 2023
Signed-off-by: Jay Deng <jayd0104@gmail.com>
@jed326 jed326 deleted the nested-scope branch March 12, 2024 22:13
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
Signed-off-by: Jay Deng <jayd0104@gmail.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
Signed-off-by: Jay Deng <jayd0104@gmail.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch bug Something isn't working flaky-test Random test failure that succeeds on second run skip-changelog
Projects
None yet
3 participants