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

Renaming RestAPIs while supporting backwards compatibility. #35

Merged
merged 3 commits into from
May 13, 2021

Conversation

saratvemulapalli
Copy link
Member

@saratvemulapalli saratvemulapalli commented May 5, 2021

Signed-off-by: Sarat Vemulapalli vemsarat@amazon.com

Description

Renaming RestAPIs from _opendistro to _plugins.

  • This PR replaces all existing REST APIs starting with _opendistro to _plugins.
    Example:
    _opendistro/_anomaly_detection/* -> _plugins/_anomaly_detection/*
  • It supports backwards compatibility as old APIs still work, but a warning message in the logs is spit out explaining the path is deprecated.
  • Tested APIs with both prefixes _opendistro and _plugins. Added integration tests to verify.

Check List

  • Commits are signed per the DCO using --signoff

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.

Signed-off-by: Sarat Vemulapalli <vemsarat@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #35 (329f872) into main (a648e86) will increase coverage by 0.12%.
The diff coverage is 91.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #35      +/-   ##
============================================
+ Coverage     79.21%   79.34%   +0.12%     
- Complexity     2681     2694      +13     
============================================
  Files           242      242              
  Lines         11025    11065      +40     
  Branches       1010     1012       +2     
============================================
+ Hits           8734     8780      +46     
+ Misses         1878     1873       -5     
+ Partials        413      412       -1     
Flag Coverage Δ Complexity Δ
plugin 79.34% <91.22%> (+0.12%) 2694.00 <12.00> (+13.00)

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

Impacted Files Coverage Δ Complexity Δ
...stroforelasticsearch/ad/AnomalyDetectorPlugin.java 95.38% <ø> (ø) 10.00 <0.00> (ø)
...search/ad/rest/RestIndexAnomalyDetectorAction.java 49.01% <75.00%> (+2.08%) 4.00 <1.00> (+1.00)
...forelasticsearch/ad/rest/AbstractSearchAction.java 40.42% <77.77%> (+15.42%) 4.00 <3.00> (+2.00)
...icsearch/ad/rest/RestAnomalyDetectorJobAction.java 57.14% <100.00%> (+7.14%) 4.00 <1.00> (+1.00)
...earch/ad/rest/RestDeleteAnomalyDetectorAction.java 60.00% <100.00%> (+6.15%) 5.00 <1.00> (+1.00)
...arch/ad/rest/RestExecuteAnomalyDetectorAction.java 26.19% <100.00%> (+3.69%) 4.00 <1.00> (+1.00)
...icsearch/ad/rest/RestGetAnomalyDetectorAction.java 48.00% <100.00%> (+9.90%) 5.00 <2.00> (+1.00)
...arch/ad/rest/RestPreviewAnomalyDetectorAction.java 20.00% <100.00%> (+4.21%) 5.00 <1.00> (+1.00)
...elasticsearch/ad/rest/RestSearchADTasksAction.java 100.00% <100.00%> (ø) 2.00 <0.00> (ø)
...earch/ad/rest/RestSearchAnomalyDetectorAction.java 100.00% <100.00%> (ø) 2.00 <0.00> (ø)
... and 7 more

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This works, but I think it's a bit too much copy-paste. I suggest modifying/wrapping/extending Route to either allow multiple paths in the constructor, or extending Route with a static method that takes a method, and a list of paths, to return a list of routes.

Signed-off-by: Sarat Vemulapalli <vemsarat@amazon.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I think "replaced" is not a great name, maybe "deprecated"? Otherwise looks ok.

@dblock dblock removed this from In Progress in OpenSearch Backward Compatibility May 12, 2021
Signed-off-by: Sarat Vemulapalli <vemsarat@amazon.com>
@saratvemulapalli saratvemulapalli marked this pull request as ready for review May 12, 2021 20:54
@dblock
Copy link
Member

dblock commented May 13, 2021

I think "replaced" is not a great name, maybe "deprecated"? Otherwise looks ok.

I wrote this when I didn't know there was support for this already in OpenSearch.

@dblock
Copy link
Member

dblock commented May 13, 2021

LGTM, but would wait for @ylwu-amzn to re-review

Copy link
Collaborator

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the change.

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.

None yet

5 participants