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

Implement toXContent on ShardOpertionFailureException #11155

Merged
merged 1 commit into from May 18, 2015

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented May 13, 2015

ShardOperationFailureException implementations alread provide structured
exception support but it's not yet exposed on the interface. This change
allows nice rendering of structured REST exceptions also if searches fail on
only a subset of the shards etc.

Closes #11017

for instance

PUT test2/test/1
{
  "foz": "bar"
}

GET _search?sort=foo

gives me now:

{
   "took": 2,
   "timed_out": false,
   "_shards": {
      "total": 7,
      "successful": 5,
      "failed": 2,
      "failures": [
         {
            "shard": 0,
            "index": "foo",
            "node": "2KRJGnOCRniCyyOcKw_ypg",
            "reason": {
               "type": "search_parse_exception",
               "reason": "Failed to parse source [{\"sort\":[{\"foo\":{}}]}]",
               "caused_by": {
                  "type": "search_parse_exception",
                  "reason": "No mapping found for [foo] in order to sort on"
               }
            }
         },
         {
            "shard": 0,
            "index": "foobar",
            "node": "2KRJGnOCRniCyyOcKw_ypg",
            "reason": {
               "type": "search_parse_exception",
               "reason": "Failed to parse source [{\"sort\":[{\"foo\":{}}]}]",
               "caused_by": {
                  "type": "search_parse_exception",
                  "reason": "No mapping found for [foo] in order to sort on"
               }
            }
         }
      ]
   },
   "hits": {
      "total": 1,
      "max_score": null,
      "hits": [
         {
            "_index": "test2",
            "_type": "test",
            "_id": "1",

similar for _msearch

GET _msearch
{"index": "test"}
{"sort": "foo"}
{"index": "_all"}
{"sort": "foo"}

now returns:

{
   "responses": [
      {
         "error": "SearchPhaseExecutionException[all shards failed]"
      },
      {
         "took": 28,
         "timed_out": false,
         "_shards": {
            "total": 12,
            "successful": 5,
            "failed": 7,
            "failures": [
               {
                  "shard": 0,
                  "index": "foo",
                  "node": "2KRJGnOCRniCyyOcKw_ypg",
                  "reason": {
                     "type": "search_parse_exception",
                     "reason": "Failed to parse source [{\"sort\": \"foo\"}]",
                     "line": 1,
                     "col": 2,
                     "caused_by": {
                        "type": "search_parse_exception",
                        "reason": "No mapping found for [foo] in order to sort on"
                     }
                  }
               },

bulk unfortunately still doesn't really work well here but it seems like a bigger thing and I wonder if we should do it at least for now

@s1monw
Copy link
Contributor Author

s1monw commented May 13, 2015

@clintongormley happy?

public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field("shard", shardId());
builder.field("index", index());
if (reason != null) {
Copy link
Member

Choose a reason for hiding this comment

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

should we keep the status as well here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah can do

@kimchy
Copy link
Member

kimchy commented May 13, 2015

left a minor comment, LGTM

@s1monw
Copy link
Contributor Author

s1monw commented May 13, 2015

I pushed the status - need to add some tests to ensure we don't loose this functionality before I push

@clintongormley
Copy link

Looking much better.

@s1monw would it be possible to group shard failures in the same way that we do for top-level exceptions?

@s1monw
Copy link
Contributor Author

s1monw commented May 15, 2015

@clintongormley I pushed a new commit adding structured exceptions to bulk as well as the grouping to all the other relevant APIs:

{
   "took": 166,
   "errors": true,
   "items": [
      {
         "index": {
            "_index": "test",
            "_type": "test",
            "_id": "1",
            "_version": 1,
            "_shards": {
               "total": 2,
               "successful": 1,
               "failed": 0
            },
            "status": 201
         }
      },
      {
         "index": {
            "_index": "test",
            "_type": "test",
            "_id": "1",
            "status": "CONFLICT",
            "error": {
               "type": "version_conflict_engine_exception",
               "reason": "[test][1]: version conflict, current [1], provided [2]",
               "shard": 3,
               "index": "test"
            }
         }
      }
   ]
}

@kimchy I changed quite some other classes - wanna do another review?

@clintongormley
Copy link

@s1monw looks awesome! Only problem is in the bulk response. The status is now being stringified to CONFLICT instead of 409 (while success still returns 200)

@s1monw
Copy link
Contributor Author

s1monw commented May 15, 2015

so we want the number @clintongormley not sure what the success comment means?

@s1monw
Copy link
Contributor Author

s1monw commented May 15, 2015

I see never mind, I fixed the stringifying problem in the last commit

ShardOperationFailureException implementations alread provide structured
exception support but it's not yet exposed on the interface. This change
allows nice rendering of structured REST exceptions also if searches fail on
only a subset of the shards etc.

Closes elastic#11017
@s1monw s1monw merged commit 8b8ba9a into elastic:master May 18, 2015
@s1monw s1monw removed the review label May 18, 2015
@lcawl lcawl added :Core/Infra/Core Core issues without another label and removed :Exceptions labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial shard failures rendered as non-structured exceptions
4 participants