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

Make MultiBucketConsumerService thread safe to use across slices during search #9047

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

neetikasinghal
Copy link
Contributor

@neetikasinghal neetikasinghal commented Aug 1, 2023

Description

callCount variable needs to be made thread safe as there is a periodic check on the CircuitBreaker in the MultiConsumerService that checks if the CB has tripped. With Concurrent Search, for each shard, there are multiple slices running on different threads and having one instance of MultiBucketConsumer. Multiple threads within the same instance of MultiBucketConsumer will try to access the callCount variable, and as of current logic that checks for CB trip after every 1024 calls to the accept function, there is possibility that if the callCount is not made thread safe, the CB trip check might never happen.

Initializing callCount as a LongAdder variable making it thread safe and also having an additional volatile` variable that tries to trip the CB for the other threads in case for one of the threads CB has already tripped.

Related Issues

Resolves #7785

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@neetikasinghal
Copy link
Contributor Author

@reta @sohami please review the changes.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



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

BUILD SUCCESSFUL in 30m 45s

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



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

BUILD SUCCESSFUL in 28m 25s

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



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

BUILD SUCCESSFUL in 32m 27s

@sohami
Copy link
Collaborator

sohami commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

@neetikasinghal There are some test failures. Can you please check those ?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

@neetikasinghal
Copy link
Contributor Author

neetikasinghal commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

@neetikasinghal There are some test failures. Can you please check those ?

@sohami thanks for calling this out.
testAllocationBucketsBreaker seemed to be failing with the changes, i have fixed that now,.MixedClusterClientYamlTestSuiteIT test seems unrelated to my changes and i have seen this failing with other prs as well, ref: https://build.ci.opensearch.org/job/gradle-check/21548/

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testWriteLargeBlob
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=cluster.allocation_explain/10_basic/cluster shard allocation explanation test with empty request}

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #9047 (53bfcc2) into main (e55dade) will increase coverage by 0.07%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 53bfcc2 differs from pull request most recent head 4a20a20. Consider uploading reports for the commit 4a20a20 to get more accurate results

@@             Coverage Diff              @@
##               main    #9047      +/-   ##
============================================
+ Coverage     71.03%   71.10%   +0.07%     
+ Complexity    57314    57309       -5     
============================================
  Files          4765     4765              
  Lines        270357   270305      -52     
  Branches      39543    39534       -9     
============================================
+ Hits         192043   192199     +156     
+ Misses        62151    61966     -185     
+ Partials      16163    16140      -23     
Files Changed Coverage Δ
...earch/aggregations/MultiBucketConsumerService.java 91.07% <100.00%> (+3.57%) ⬆️

... and 523 files with indirect coverage changes

@sohami
Copy link
Collaborator

sohami commented Aug 3, 2023

@neetikasinghal Can you please rebase your branch ?

…ng search

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
@neetikasinghal
Copy link
Contributor Author

neetikasinghal commented Aug 3, 2023

@neetikasinghal Can you please rebase your branch ?

@sohami Done

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

@sohami
Copy link
Collaborator

sohami commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

Known Flaky test: #9034

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



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

BUILD SUCCESSFUL in 28m 21s

@sohami sohami added the backport 2.x Backport to 2.x branch label Aug 4, 2023
@sohami sohami merged commit 24595c9 into opensearch-project:main Aug 4, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-9047-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 24595c9fbace3516e886137058f3a8b992937652
# Push it to GitHub
git push --set-upstream origin backport/backport-9047-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-9047-to-2.x.

@sohami
Copy link
Collaborator

sohami commented Aug 4, 2023

@neetikasinghal Backport failed. Can you please do it manually

neetikasinghal added a commit to neetikasinghal/OpenSearch that referenced this pull request Aug 4, 2023
…ng search (opensearch-project#9047)

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
(cherry picked from commit 24595c9)
neetikasinghal added a commit to neetikasinghal/OpenSearch that referenced this pull request Aug 4, 2023
…ng search (opensearch-project#9047)

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
(cherry picked from commit 24595c9)
neetikasinghal added a commit to neetikasinghal/OpenSearch that referenced this pull request Aug 4, 2023
…ng search (opensearch-project#9047)

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
sohami pushed a commit that referenced this pull request Aug 4, 2023
…ng search (#9047) (#9110)

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Sep 12, 2023
…ng search (opensearch-project#9047)

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>
brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023
…ng search (opensearch-project#9047)

Signed-off-by: Neetika Singhal <neetiks@amazon.com>
Signed-off-by: Ivan Brusic <ivan.brusic@flocksafety.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…ng search (opensearch-project#9047)

Signed-off-by: Neetika Singhal <neetiks@amazon.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Concurrent Segment Search] Make MultiBucketConsumerService thread safe to use across slices during search
3 participants