-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Conversation
@clintongormley happy? |
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.field("shard", shardId()); | ||
builder.field("index", index()); | ||
if (reason != null) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah can do
left a minor comment, LGTM |
I pushed the status - need to add some tests to ensure we don't loose this functionality before I push |
Looking much better. @s1monw would it be possible to group shard failures in the same way that we do for top-level exceptions? |
@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? |
@s1monw looks awesome! Only problem is in the bulk response. The status is now being stringified to |
so we want the number @clintongormley not sure what the success comment means? |
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
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
gives me now:
similar for
_msearch
now returns:
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