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

Abstract AsyncShardFetch cache to allow restructuring for other caching strategies #12441

Merged
merged 14 commits into from
Mar 11, 2024

Conversation

amkhar
Copy link
Contributor

@amkhar amkhar commented Feb 23, 2024

Description

Abstract AsyncShardFetch cache to allow restructuring for other caching strategies.
Currently cache is like private final Map<String, NodeEntry<T>> cache = new HashMap<>();
This is can not support other strategies. So abstracting out this implementation in a separate class will help other caching strategies to be implemented easily.

Related Issues

Resolves ##12440

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.

@amkhar amkhar changed the title Abstract AsyncShardFetch cache to allow restructuring for batch mode Abstract AsyncShardFetch cache to allow restructuring for other caching strategies Feb 23, 2024
Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Feb 23, 2024

Compatibility status:

Checks if related components are compatible with change d7903f7

Incompatible components

Skipped components

Compatible components

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

Signed-off-by: Aman Khare <[email protected]>
Copy link
Contributor

❌ Gradle check result for 608c8ae: 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?

Signed-off-by: Aman Khare <[email protected]>
Copy link
Contributor

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

Copy link
Contributor

❌ Gradle check result for 0daa63e: 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?

Copy link
Contributor

❕ Gradle check result for 8383c63: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.RemoteStoreStatsIT.testDownloadStatsCorrectnessSinglePrimaryMultipleReplicaShards

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 Feb 28, 2024

Codecov Report

Attention: Patch coverage is 81.33333% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 71.35%. Comparing base (2b17902) to head (d7903f7).

Files Patch % Lines
...a/org/opensearch/gateway/AsyncShardFetchCache.java 78.33% 10 Missing and 16 partials ⚠️
...n/java/org/opensearch/gateway/AsyncShardFetch.java 93.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #12441      +/-   ##
============================================
- Coverage     71.43%   71.35%   -0.08%     
- Complexity    59960    59982      +22     
============================================
  Files          4984     4985       +1     
  Lines        282247   282275      +28     
  Branches      40952    40946       -6     
============================================
- Hits         201617   201415     -202     
- Misses        63954    64198     +244     
+ Partials      16676    16662      -14     

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

@amkhar
Copy link
Contributor Author

amkhar commented Feb 28, 2024

❕ Gradle check result for 8383c63: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.RemoteStoreStatsIT.testDownloadStatsCorrectnessSinglePrimaryMultipleReplicaShards

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

Flaky test link : #10152

@amkhar amkhar marked this pull request as ready for review March 1, 2024 06:52
Copy link
Contributor

github-actions bot commented Mar 6, 2024

❕ Gradle check result for 91e094a: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testMultiGetWithNetworkDisruption_FailOpenEnabled

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

@sachinpkale
Copy link
Member

Changes LGTM but there are no tests added for changes in this PR. Are they already covered?

@amkhar
Copy link
Contributor Author

amkhar commented Mar 7, 2024

Changes LGTM but there are no tests added for changes in this PR. Are they already covered?

Thanks @sachinpkale !
Yes, AsyncShardFetchTests class already covers all the tests for existing implementation. For new implementations, tests are being added in https://github.com/opensearch-project/OpenSearch/pull/12504/files#diff-4c4d5a289cb13787ecdcf2d6ae890c7f21a54aa15086d4acf2c5894a1d332420

This PR is just for abstracting out the code from 1 single file to 3 different files for better extensibility.

Aman Khare added 2 commits March 8, 2024 16:29
…se class to make it specific

Signed-off-by: Aman Khare <[email protected]>
Signed-off-by: Aman Khare <[email protected]>
Copy link
Contributor

github-actions bot commented Mar 8, 2024

❌ Gradle check result for 9fe6a07: 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?

Signed-off-by: Aman Khare <[email protected]>
Copy link
Contributor

github-actions bot commented Mar 8, 2024

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

@sachinpkale
Copy link
Member

Please fix the build.

Copy link
Contributor

✅ Gradle check result for d7903f7: SUCCESS

@sachinpkale sachinpkale merged commit b15cb0c into opensearch-project:main Mar 11, 2024
31 of 33 checks passed
@sachinpkale sachinpkale added the backport 2.x Backport to 2.x branch label Mar 11, 2024
@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:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-12441-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b15cb0c52be59ceaad16f2f47b3af3018a563211
# Push it to GitHub
git push --set-upstream origin backport/backport-12441-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

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

amkhar added a commit to amkhar/OpenSearch that referenced this pull request Mar 12, 2024
…ng strategies (opensearch-project#12441)

* Abstract AsyncShardFetch cache to allow restructuring for batch mode

Signed-off-by: Aman Khare <[email protected]>
amkhar added a commit to amkhar/OpenSearch that referenced this pull request Mar 14, 2024
…ng strategies (opensearch-project#12441)

* Abstract AsyncShardFetch cache to allow restructuring for batch mode

Signed-off-by: Aman Khare <[email protected]>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
…ng strategies (opensearch-project#12441)

* Abstract AsyncShardFetch cache to allow restructuring for batch mode

Signed-off-by: Aman Khare <[email protected]>
amkhar added a commit to amkhar/OpenSearch that referenced this pull request Mar 19, 2024
…ng strategies (opensearch-project#12441)

* Abstract AsyncShardFetch cache to allow restructuring for batch mode

Signed-off-by: Aman Khare <[email protected]>
(cherry picked from commit b15cb0c)
shwetathareja pushed a commit that referenced this pull request Mar 19, 2024
…ng strategies (#12441) (#12761)

* Abstract AsyncShardFetch cache to allow restructuring for batch mode

Signed-off-by: Aman Khare <[email protected]>
(cherry picked from commit b15cb0c)
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…ng strategies (opensearch-project#12441)

* Abstract AsyncShardFetch cache to allow restructuring for batch mode

Signed-off-by: Aman Khare <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants