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

Remove redundant field from GetDecommissionStateResponse #4751

Merged
merged 12 commits into from
Oct 18, 2022

Conversation

imRishN
Copy link
Member

@imRishN imRishN commented Oct 12, 2022

Signed-off-by: Rishab Nahata rnnahata@amazon.com

Description

This PR tries to remove the redundant field from GET api to retrieve decommission state

Before the fix -

curl -X GET "localhost:9202/_cluster/decommission/awareness/_status?pretty"

{
	"awareness" : {
		"zone" : "zone-1"
	},
	"status" : "SUCCESSFUL"
}

After the fix -

~                                                                                                                                                                                                                  03:23:31
> curl -X GET "localhost:9200/_cluster/decommission/awareness/zone/_status?pretty"
{ }

~                                                                                                                                                                                                                  03:23:36
> curl -X PUT "localhost:9201/_cluster/decommission/awareness/zone/zone-2?pretty"
{
  "acknowledged" : true
}

~                                                                                                                                                                                                                  03:23:58
> curl -X GET "localhost:9200/_cluster/decommission/awareness/zone/_status?pretty"
{
  "zone-2" : "SUCCESSFUL"
}

~                                                                                                                                                                                                                  03:24:03
> curl -X GET "localhost:9200/_cluster/decommission/awareness/random/_status?pretty"
{ }

~                                                                                                                                                                                                                  03:24:11
> curl -X GET "localhost:9202/_cluster/decommission/awareness/random/_status?pretty"
{ }

~                                                                                                                                                                                                                  03:24:18
> curl -X GET "localhost:9202/_cluster/decommission/awareness/zone/_status?pretty"
{
  "zone-2" : "SUCCESSFUL"
}

~                                                                                                                                                                                                                  03:24:24
> curl -X GET "localhost:9202/_cluster/decommission/awareness/_status?pretty"
{
  "error" : "no handler found for uri [/_cluster/decommission/awareness/_status] and method [GET]"
}

Issues Resolved

#4750

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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: Rishab Nahata <rnnahata@amazon.com>
@imRishN imRishN requested review from a team and reta as code owners October 12, 2022 17:23
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sachinpkale
Copy link
Member

Overall changes look good. Can you please update the PR description with API response before and after the change?

Also, the build is failing. Please fix.

…i-fix

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

Merging #4751 (89b107b) into main (5d7d83e) will increase coverage by 0.21%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #4751      +/-   ##
============================================
+ Coverage     70.60%   70.82%   +0.21%     
- Complexity    57603    57734     +131     
============================================
  Files          4675     4675              
  Lines        276925   276916       -9     
  Branches      40347    40340       -7     
============================================
+ Hits         195517   196114     +597     
+ Misses        65156    64548     -608     
- Partials      16252    16254       +2     
Impacted Files Coverage Δ
...reness/get/GetDecommissionStateRequestBuilder.java 0.00% <0.00%> (ø)
...eness/get/TransportGetDecommissionStateAction.java 23.07% <0.00%> (-1.93%) ⬇️
.../org/opensearch/client/support/AbstractClient.java 32.54% <ø> (+0.23%) ⬆️
.../admin/cluster/RestGetDecommissionStateAction.java 42.85% <25.00%> (-17.15%) ⬇️
...on/awareness/get/GetDecommissionStateResponse.java 42.85% <47.36%> (-1.92%) ⬇️
...ion/awareness/get/GetDecommissionStateRequest.java 82.35% <83.33%> (+82.35%) ⬆️
.../java/org/opensearch/node/NodeClosedException.java 50.00% <0.00%> (-50.00%) ⬇️
...ch/transport/ReceiveTimeoutTransportException.java 50.00% <0.00%> (-50.00%) ⬇️
...nsearch/rest/action/cat/RestCatRecoveryAction.java 43.61% <0.00%> (-42.56%) ⬇️
...ations/bucket/terms/heuristic/PercentageScore.java 17.39% <0.00%> (-34.79%) ⬇️
... and 471 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@saikaranam-amazon saikaranam-amazon left a comment

Choose a reason for hiding this comment

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

LGTM - Can we change the RFC to reflect the latest contract?

Copy link
Collaborator

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

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

Lets add support for per attribute GET

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@imRishN
Copy link
Member Author

imRishN commented Oct 17, 2022

@Bukhtawar as discussed, updated the Request path and Response for the GET request. Now, the path will need to have attributeName specified. The response for a valid request will be attributeValue: status.

This is also in consistent with how the WeightedRouting GET request and response implements it.

Thanks for helping simplifying it.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

}

@Override
public ActionRequestValidationException validate() {
return null;
ActionRequestValidationException validationException = null;
if (attributeName == null || Strings.isEmpty(attributeName)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Strings.isEmpty

@Bukhtawar Bukhtawar merged commit f3d038f into opensearch-project:main Oct 18, 2022
imRishN added a commit to imRishN/OpenSearch that referenced this pull request Oct 28, 2022
…project#4751)

* Add attribute name to query param and simplify GetDecommissionStateResponse

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Bukhtawar pushed a commit that referenced this pull request Nov 2, 2022
* Add DecommissionService and helper to execute awareness attribute decommissioning #4084
* Add APIs (GET/PUT) to decommission awareness attribute #4261
* Controlling discovery for decommissioned nodes #4590
* Fix decommission status update to non leader nodes #4800
* Remove redundant field from GetDecommissionStateResponse #4751
* Service Layer changes for Recommission API #4320
* Recommission api level support #4604
* Fix bug in AwarenessAttributeDecommissionIT #4822

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
imRishN added a commit to imRishN/OpenSearch that referenced this pull request Nov 3, 2022
* Add DecommissionService and helper to execute awareness attribute decommissioning opensearch-project#4084
* Add APIs (GET/PUT) to decommission awareness attribute opensearch-project#4261
* Controlling discovery for decommissioned nodes opensearch-project#4590
* Fix decommission status update to non leader nodes opensearch-project#4800
* Remove redundant field from GetDecommissionStateResponse opensearch-project#4751
* Service Layer changes for Recommission API opensearch-project#4320
* Recommission api level support opensearch-project#4604
* Fix bug in AwarenessAttributeDecommissionIT opensearch-project#4822

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
imRishN added a commit to imRishN/OpenSearch that referenced this pull request Nov 3, 2022
* Add DecommissionService and helper to execute awareness attribute decommissioning opensearch-project#4084
* Add APIs (GET/PUT) to decommission awareness attribute opensearch-project#4261
* Controlling discovery for decommissioned nodes opensearch-project#4590
* Fix decommission status update to non leader nodes opensearch-project#4800
* Remove redundant field from GetDecommissionStateResponse opensearch-project#4751
* Service Layer changes for Recommission API opensearch-project#4320
* Recommission api level support opensearch-project#4604
* Fix bug in AwarenessAttributeDecommissionIT opensearch-project#4822

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Bukhtawar pushed a commit that referenced this pull request Nov 3, 2022
* Awareness attribute decommission backports (#4970)
* Add DecommissionService and helper to execute awareness attribute decommissioning #4084
* Add APIs (GET/PUT) to decommission awareness attribute #4261
* Controlling discovery for decommissioned nodes #4590
* Fix decommission status update to non leader nodes #4800
* Remove redundant field from GetDecommissionStateResponse #4751
* Service Layer changes for Recommission API #4320
* Recommission api level support #4604
* Fix bug in AwarenessAttributeDecommissionIT #4822

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
ashking94 pushed a commit to ashking94/OpenSearch that referenced this pull request Nov 7, 2022
…project#4751)

* Add attribute name to query param and simplify GetDecommissionStateResponse

Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
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.

6 participants