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

Adding remote index and multi-index checks in validation #1290

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

amitgalitz
Copy link
Member

Description

Adds ability to validate multiple indices/aliases and remote indices during validation, rest of the remote indexing feature actually works out of the box on any search action that is done.

  1. Validation for timefield works based on the fact that all indices given should have the given timestamp or we fail
  2. Validation for categorical field is similar that we required all indices to have the given categorical field

Tasks left over:

  • Writing a few more integ tests and fixing one more broken unit tests

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • 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.

Comment on lines 42 to 43
logger.info("clusterName1: " + clusterName);
logger.info("clusterService.getClusterName().value(): " + clusterService.getClusterName().value());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these logs still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

Comment on lines 68 to 69
String clusterName = parseClusterName(index);
String indexName = parseIndexName(index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make one call and return a Pair instead of calling similar logic twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

Signed-off-by: Amit Galitzky <[email protected]>
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 89.89899% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.46%. Comparing base (3f0fc8c) to head (988ebfb).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
.../rest/handler/AbstractTimeSeriesActionHandler.java 89.33% 4 Missing and 4 partials ⚠️
...earch/timeseries/util/CrossClusterConfigUtils.java 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1290      +/-   ##
============================================
+ Coverage     71.83%   77.46%   +5.63%     
- Complexity     4898     5445     +547     
============================================
  Files           518      533      +15     
  Lines         22879    23319     +440     
  Branches       2245     2310      +65     
============================================
+ Hits          16434    18063    +1629     
+ Misses         5410     4199    -1211     
- Partials       1035     1057      +22     
Flag Coverage Δ
plugin 77.46% <89.89%> (+5.63%) ⬆️

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

Files with missing lines Coverage Δ
...opensearch/timeseries/constant/CommonMessages.java 97.50% <100.00%> (+0.06%) ⬆️
...opensearch/timeseries/rest/RestValidateAction.java 86.66% <ø> (+60.00%) ⬆️
...s/transport/BaseValidateConfigTransportAction.java 72.09% <ø> (+0.32%) ⬆️
...earch/timeseries/util/CrossClusterConfigUtils.java 91.30% <91.30%> (ø)
.../rest/handler/AbstractTimeSeriesActionHandler.java 95.44% <89.33%> (+30.63%) ⬆️

... and 104 files with indirect coverage changes

@amitgalitz amitgalitz merged commit ec16d53 into opensearch-project:main Sep 4, 2024
20 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 4, 2024
* Adding remote index and multi index checks in validation

Signed-off-by: Amit Galitzky <[email protected]>

* adding more tests

Signed-off-by: Amit Galitzky <[email protected]>

---------

Signed-off-by: Amit Galitzky <[email protected]>
(cherry picked from commit ec16d53)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
jackiehanyang pushed a commit that referenced this pull request Sep 4, 2024
* Adding remote index and multi index checks in validation



* adding more tests



---------


(cherry picked from commit ec16d53)

Signed-off-by: Amit Galitzky <[email protected]>
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>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 4, 2024
* Adding remote index and multi index checks in validation

Signed-off-by: Amit Galitzky <[email protected]>

* adding more tests

Signed-off-by: Amit Galitzky <[email protected]>

---------

Signed-off-by: Amit Galitzky <[email protected]>
(cherry picked from commit ec16d53)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
amitgalitz pushed a commit that referenced this pull request Sep 5, 2024
* Adding remote index and multi index checks in validation



* adding more tests



---------


(cherry picked from commit ec16d53)

Signed-off-by: Amit Galitzky <[email protected]>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants