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

[Extensions] Improves performance by replacing cluster state calls for index existence with exists()/aliasExists() API calls #919

Merged
merged 3 commits into from Jun 5, 2023

Conversation

joshpalis
Copy link
Member

Description

Companion SDK PR : opensearch-project/opensearch-sdk-java#802 (SDK PR must be merged first)

Replaces the following cluster state calls to check if an index/alias exists with the SDKIndicesClient exist() and existsAlias() methods :

  • state().getRoutingTable().hasIndex(indexName)
  • state().metadata().hasIndex(indexName)
  • state().metadata().indices().containsKey(indexName)
  • state().metadata().hasAlias(alias)

Issues Resolved

Part of opensearch-project/opensearch-sdk-java#674

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.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with questions.


Boolean existsResponse = existsFuture
.orTimeout(AnomalyDetectorSettings.REQUEST_TIMEOUT.get(environmentSettings).getMillis(), TimeUnit.MILLISECONDS)
.join();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need any exception handling here, even if it's just catching and rethrowing with some more information? OK if the answer is no, just wondering what the user experience will be.

(Repeat comment for most of these calls.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think its necessary as the only way this request fails is by timing out, since even if the index/alias does not exist, it would follow the happy path (for lack of a better word). There's no other information to provide.

@joshpalis
Copy link
Member Author

Checks are failing since SDK main has failed to publish to sonatype. See the error here

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except @dbwiddis comments. Rest LGTM

…tate and replaces with exists()/aliasExists() API

Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #919 (9e7da8f) into feature/extensions (c20e72d) will decrease coverage by 0.39%.
The diff coverage is 10.98%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##             feature/extensions     #919      +/-   ##
========================================================
- Coverage                 43.32%   42.94%   -0.39%     
+ Complexity                 2433     2420      -13     
========================================================
  Files                       301      301              
  Lines                     18065    18139      +74     
  Branches                   1872     1872              
========================================================
- Hits                       7826     7789      -37     
- Misses                     9656     9772     +116     
+ Partials                    583      578       -5     
Flag Coverage Δ
plugin 42.94% <10.98%> (-0.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../ad/rest/handler/AnomalyDetectorActionHandler.java 5.12% <0.00%> (-1.54%) ⬇️
...ransport/DeleteAnomalyDetectorTransportAction.java 0.00% <0.00%> (ø)
...transport/StatsAnomalyDetectorTransportAction.java 0.00% <0.00%> (ø)
...c/main/java/org/opensearch/ad/util/IndexUtils.java 1.49% <0.00%> (-0.64%) ⬇️
...c/main/java/org/opensearch/ad/util/ParseUtils.java 53.47% <20.00%> (-6.03%) ⬇️
...opensearch/ad/indices/AnomalyDetectionIndices.java 13.16% <28.57%> (+0.91%) ⬆️

... and 5 files with indirect coverage changes

@joshpalis joshpalis merged commit 56b038f into opensearch-project:feature/extensions Jun 5, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants