Make feature reset API response more informative #71240

williamrandolph · 2021-04-02T15:54:17Z

Previously, the Reset API returned a list of which feature states it tried to reset, along with success and failure messages. However, the overall return status was always success (200 OK), even if an individual feature state failed. (For example, if wildcard deletions were disabled for associated indices #71150.)

This PR changes the Reset API so that it uses different response codes for different situations: 200 OK if all reset operations succeed, 207 MULTI STATUS if some operations succeed and others fail, and 500 INTERNAL SERVER ERROR if all of the reset operations fail. Clients should check the response code from this API.

The API response now includes formatted stack traces when reset operation fails.

add documentation about the possible 207 response

Fixes #71178

elasticmachine · 2021-04-02T15:54:20Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

jaymode · 2021-04-02T21:05:58Z

Hmm, I wonder if this is a prime example for using an HTTP 207 response code as discussed in #60442? It seems like it to me where the behavior of the API is desirable so we know all of the errors rather than the first and have to play whack a mole. Maybe we should instead fix the consumers of the API to check for failures? What do you think?

williamrandolph · 2021-04-02T21:39:04Z

That seems like a good approach here, and I'll see if I can get it working quickly. Otherwise it might be worth making a new issue for it so that we can get some kind of fail-fast behavior into the tests.

Previously, the ResetFeatureStateStatus object captured its status in a String, which meant that if we wanted to know if something succeeded or failed, we'd have to parse information out of the string. This isn't a good way of doing things. I've introduced a SUCCESS/FAILURE enum for status constants, and added a check for failures in the transport action. Instead of throwing the first exception that comes back from the resets, we now throw an exception that shows a complete response with all successes and failures. We still "fail fast" when we can't reset the features, but now the user won't have to iterate in order to find all the errors. The response message could probably be improved from here. I've got some todos in the code for reminders.

williamrandolph · 2021-04-21T20:40:45Z

...r/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java

+            new RestToXContentListener<>(restChannel) {
+                @Override
+                protected RestStatus getStatus(ResetFeatureStateResponse response) {
+                    if (response.hasAllFailures()) {


So I've got ES returning a 500 if all resets fail, a 207 for mixed responses, and a 200 if all goes well. Does this make sense?

williamrandolph · 2021-04-22T16:23:26Z

I think these test failures have to do with this branch, so I'm looking into them.

jaymode

Looking pretty good. I left some comments.

...nt/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java

server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java

...ava/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java

jaymode

LGTM. Please make sure to backport to 7.x and the 7.13 branches so this makes it into the next 7.13 BC.

...nt/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java

Co-authored-by: Jay Modi <[email protected]>

Previously, the ResetFeatureStateStatus object captured its status in a String, which meant that if we wanted to know if something succeeded or failed, we'd have to parse information out of the string. This isn't a good way of doing things. I've introduced a SUCCESS/FAILURE enum for status constants, and added a check for failures in the transport action. We return a 207 if some but not all reset actions fail, and for every failure, we also return information about the exception or error that caused it. Co-authored-by: Jay Modi <[email protected]>

* Make feature reset API response more informative (#71240) Previously, the ResetFeatureStateStatus object captured its status in a String, which meant that if we wanted to know if something succeeded or failed, we'd have to parse information out of the string. This isn't a good way of doing things. I've introduced a SUCCESS/FAILURE enum for status constants, and added a check for failures in the transport action. We return a 207 if some but not all reset actions fail, and for every failure, we also return information about the exception or error that caused it. * Fix 7.x backport compilation issues * Feature reset integration test should tolerate failed resets (#72326) Co-authored-by: Jay Modi <[email protected]>

…stic#72332) * Make feature reset API response more informative (elastic#71240) Previously, the ResetFeatureStateStatus object captured its status in a String, which meant that if we wanted to know if something succeeded or failed, we'd have to parse information out of the string. This isn't a good way of doing things. I've introduced a SUCCESS/FAILURE enum for status constants, and added a check for failures in the transport action. We return a 207 if some but not all reset actions fail, and for every failure, we also return information about the exception or error that caused it. * Fix 7.x backport compilation issues * Feature reset integration test should tolerate failed resets (elastic#72326) Co-authored-by: Jay Modi <[email protected]>

williamrandolph · 2021-04-28T00:00:10Z

Backport to 7.x: #72332
Backport to 7.13: #72341

* Add docs for feature reset API (#71759) * Make feature reset API response more informative (#71240) Previously, the ResetFeatureStateStatus object captured its status in a String, which meant that if we wanted to know if something succeeded or failed, we'd have to parse information out of the string. This isn't a good way of doing things. I've introduced a SUCCESS/FAILURE enum for status constants, and added a check for failures in the transport action. We return a 207 if some but not all reset actions fail, and for every failure, we also return information about the exception or error that caused it. * Fix 7.x backport compilation issues * Feature reset integration test should tolerate failed resets (#72326) Co-authored-by: debadair <[email protected]> Co-authored-by: Jay Modi <[email protected]>

Reset API should fail fast when there is an error

7855587

williamrandolph added >non-issue :Core/Infra/Core Core issues without another label v8.0.0 v7.13.0 labels Apr 2, 2021

williamrandolph requested a review from jaymode April 2, 2021 15:54

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 2, 2021

Use admin credentials with reset API

9a8f090

williamrandolph added 7 commits April 2, 2021 21:07

Merge branch 'master' into reset-api-fail-fast

99b863a

Merging changes from master

282a38b

Merge branch 'master' into reset-api-fail-fast

db18cc7

Return a 207 if some but not all reset actions fail

3123225

Merge branch 'master' into reset-api-fail-fast

bad2600

Serialize stack trace in usable format

6e30065

williamrandolph requested review from jaymode and removed request for jaymode April 21, 2021 20:39

williamrandolph commented Apr 21, 2021

View reviewed changes

williamrandolph added 3 commits April 21, 2021 16:48

Merge branch 'master' into reset-api-fail-fast

5a3bdc8

Reduce number of iterations over response set

b039ca9

Merge branch 'master' into reset-api-fail-fast

d4eacc2

williamrandolph added 3 commits April 22, 2021 14:50

Add optional exception to HLRC reset feature state response

e6113a3

Add tests for xcontent serialization

deffae3

Make method name more specific

f3f29f0

williamrandolph changed the title ~~Reset API should fail fast when there is an error~~ Make feature reset API response more informative Apr 23, 2021

Add a line to the docs about response codes

d2c1b45

Use constructing object parser to handle exception

cd047f2

jaymode requested changes Apr 26, 2021

View reviewed changes

williamrandolph added 5 commits April 26, 2021 15:12

Improve naming in ResetFeaturesResponse and add javadoc

83f2019

Use concurrency-safe accessors for static variable

19e4842

Respond to PR feedback

d277a24

Pity me and my XContent woes

9e2ab6f

Merge branch 'master' into reset-api-fail-fast

e5f0332

williamrandolph requested a review from jaymode April 26, 2021 23:10

jaymode added the v7.14.0 label Apr 27, 2021

jaymode approved these changes Apr 27, 2021

View reviewed changes

...nt/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java Outdated Show resolved Hide resolved

Improve javadoc

ff5a9d8

Co-authored-by: Jay Modi <[email protected]>

williamrandolph merged commit fc7c06d into elastic:master Apr 27, 2021

williamrandolph added the backport pending label Apr 27, 2021

williamrandolph mentioned this pull request Apr 27, 2021

Make feature reset API response more informative (#71240) #72341

Merged

jaymode removed the backport pending label Apr 28, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

williamrandolph deleted the reset-api-fail-fast branch May 23, 2022 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make feature reset API response more informative #71240

Make feature reset API response more informative #71240

williamrandolph commented Apr 2, 2021 •

edited

Loading

elasticmachine commented Apr 2, 2021

jaymode commented Apr 2, 2021 •

edited

Loading

williamrandolph commented Apr 2, 2021

williamrandolph Apr 21, 2021 •

edited

Loading

williamrandolph commented Apr 22, 2021

jaymode left a comment

jaymode left a comment

williamrandolph commented Apr 28, 2021

Make feature reset API response more informative #71240

Make feature reset API response more informative #71240

Conversation

williamrandolph commented Apr 2, 2021 • edited Loading

elasticmachine commented Apr 2, 2021

jaymode commented Apr 2, 2021 • edited Loading

williamrandolph commented Apr 2, 2021

williamrandolph Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

williamrandolph commented Apr 22, 2021

jaymode left a comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

williamrandolph commented Apr 28, 2021

williamrandolph commented Apr 2, 2021 •

edited

Loading

jaymode commented Apr 2, 2021 •

edited

Loading

williamrandolph Apr 21, 2021 •

edited

Loading