-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Avoid making further transport calls if paginationStrategy outputs empty entities #16444
Avoid making further transport calls if paginationStrategy outputs empty entities #16444
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16444 +/- ##
============================================
+ Coverage 72.04% 72.09% +0.05%
- Complexity 65020 65056 +36
============================================
Files 5313 5313
Lines 303356 303365 +9
Branches 43900 43902 +2
============================================
+ Hits 218551 218718 +167
+ Misses 66905 66728 -177
- Partials 17900 17919 +19 ☔ View full report in Codecov by Sentry. |
…entities Signed-off-by: Harsh Garg <gkharsh@amazon.com>
523acf6
to
b328dbc
Compare
Signed-off-by: Harsh Garg <gkharsh@amazon.com>
❌ Gradle check result for 016fcaf: 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? |
server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Harsh Garg <gkharsh@amazon.com>
2099d92
to
40cc55a
Compare
@gargharsh3134 can we add test for it? |
❌ Gradle check result for a44870a: 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: Harsh Garg <gkharsh@amazon.com>
a44870a
to
6d0acfc
Compare
@shwetathareja I did discuss this earlier with @rajiv-kv. But, we don't have any UTs for RestLayer and It won't be possible to assert this info in an Integ Test, that's why I just added functional testing details. It is the behaviour of each TransportNodesAction, that if indices in the request are not set, we end up fetching everything, Not really sure how to have this in tests. Would you have any suggestions? |
❕ Gradle check result for 6d0acfc: 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. |
Opened an issue to track addition of integ tests inside TransportActions -> #16454 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving it so that it can make it to 2.18. Test will be added as fast follow immediately.
9a476b6
into
opensearch-project:main
Description
For paginated
_list/indices
and_list/shards
APIs, if their corresponding strategies don't output any entities, theindicesToBeQueried
gets set to empty and any subsequentIndicesStatsTransport
action ends up fetching stats for all the indices. Change in this PR addresses and optimizes such scenarios.Functional Testing:
Added following additional info logs just for the testing purpose (no need to include such logs in main):
local cluster's indices:
Before making changes, querying for a wildcard which doesn't match any index was leading to all the index stats being fetched unnecessarily.
After making the changes, stats call is not being made:
Related Issues
Resolves #16448
Check List
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.