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

Core: Pick inner most parse exception as root cause #30270

Merged
merged 1 commit into from May 1, 2018

Conversation

Projects
None yet
4 participants
@nik9000
Copy link
Contributor

commented Apr 30, 2018

Just like ElasticsearchException, the inner most
XContentParseException tends to contain the root cause of the
exception and show be show to the user in the root_cause field.

The effectively undoes most of the changes that #29373 made to the
root_cause for parsing exceptions. The type field still changes from
parse_exception to x_content_parse_exception, but this seems like a
fairly safe change.

ElasticsearchWrapperException looks tempting to implement this but
the behavior isn't quite right. ElasticsearchWrapperExceptions are
entirely unwrapped until the cause no longer
implements ElasticsearchWrapperException but XContentParseException
should be unwrapped until its cause is no longer an
XContentParseException but no further. In other words,
ElasticsearchWrapperException are unwrapped one step too far.

Closes #30261

Core: Pick inner most parse exception as root cause
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2018

return ((ElasticsearchException) ex).guessRootCauses();
}
if (ex instanceof XContentParseException) {
/*

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 30, 2018

Author Contributor

I feel like if we get another exception like this one we should try to build something a little more generic and less "heuristics that make the exception look good". But XContentParseException isn't really the same thing as ElasticsearchException`. It is similar, but different enough that I don't think I could boil out the generic bits without this getting wonky.

@@ -19,7 +19,11 @@

package org.elasticsearch;

/**
* An exception that is meant to be "unwrapped" when sent back to the user

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 30, 2018

Author Contributor

This one turned out not to be useful but I researched it so I figured I'd add the javadocs.

@@ -124,13 +126,13 @@ public void testGuessRootCause() {
} else {
rootCauses = ElasticsearchException.guessRootCauses(randomBoolean() ? new RemoteTransportException("remoteboom", ex) : ex);
}
assertEquals(ElasticsearchException.getExceptionName(rootCauses[0]), "parsing_exception");

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 30, 2018

Author Contributor

These were all backwards....

@dakrone
Copy link
Member

left a comment

LGTM

@nik9000 nik9000 merged commit 99b98fa into elastic:master May 1, 2018

3 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

Thanks for reviewing @dakrone! I've merged and I'll backport now!

nik9000 added a commit that referenced this pull request May 1, 2018

Core: Pick inner most parse exception as root cause (#30270)
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261

nik9000 added a commit that referenced this pull request May 1, 2018

Core: Pick inner most parse exception as root cause (#30270)
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.