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

HLRC: Fix strict setting exception handling #37247

Merged
merged 9 commits into from Jan 30, 2019

Conversation

Projects
None yet
7 participants
@hub-cap
Copy link
Contributor

commented Jan 9, 2019

The LLRC's exception handling for strict mode was previously throwing an
exception the HLRC assumed was an error response. This is not the case
if the result is valid in strict mode, as it will return the proper
response wrapped in an exception with warnings. This commit fixes the
HLRC such that it no longer spews if it encounters a strict LLRC
response.

Closes #37090

HLRC: Fix strict setting exception handling
The LLRC's exception handling for strict mode was previously throwing an
exception the HLRC assumed was an error response. This is not the case
if the result is valid in strict mode, as it will return the proper
response wrapped in an exception with warnings. This commit fixes the
HLRC such that it no longer spews if it encounters a strict LLRC
response.

Closes #37090
@elasticmachine

This comment has been minimized.

Copy link

commented Jan 9, 2019

@hub-cap

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@elasticmachine please run gradle build tests 1


if (responseException instanceof WarningFailureException) {
elasticsearchException = new ElasticsearchStatusException("Warnings/Deprecations caused response to fail", restStatus,
responseException);

This comment has been minimized.

Copy link
@javanna

javanna Jan 10, 2019

Member

I find using ElasticsearchStatusException almost as weird as using ResponseException. That is because such exceptions are meant for error status codes, while warnings can come back with any response.

I would prefer if we could have a separate catch for the warnings case, with a different exception that does not extend ResponseException. Maybe we could add a new exception to the low-level client that does not extend ResponseException, and let the high-level client bubble it up as-is? Not sure what I may be missing that prevents us from going down this route...

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 11, 2019

Contributor

I think all we have to do for that is to make the new exception not extend ResponseException. It might have to extend RuntimeException so we don't break compilation. Which'd be fine with me.

This comment has been minimized.

Copy link
@javanna

javanna Jan 11, 2019

Member

you are right Nik, probably we do not even need a catch for the new exception, just let it bubble up.

This comment has been minimized.

Copy link
@hub-cap

hub-cap Jan 14, 2019

Author Contributor

The only reason I used the existing ElasticsearchStatusException was because that is what was thrown before. If you take a look at https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java#L1670, it is the method that sync/async both call when a ResponseException is thrown from the LLRC. Im not sure there is a ton of value wrapping either of these in ElasticsearchStatusException. I think removing this would be a breaking change tho?

This comment has been minimized.

Copy link
@hub-cap

hub-cap Jan 14, 2019

Author Contributor

and to @nik9000 point, if we just throw another exception, it works great for sync, but for async it throws the following

UncategorizedExecutionException[Failed execution
]; nested: ExecutionException[org.elasticsearch.client.DeprecationException

That said, ofc we can make it do whatever we want. Id still lean toward 1) doing nothing different and still throwing a ElasticsearchStatusException, or 2) unwrapping both the ResponseException and WarningFailureException and throwing them w/o the ElasticsearchStatusException (breaking?)

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 14, 2019

Contributor

I think that removing the ElasticsearchStatusException wrapped from the ResponseException thrown would be a bad breaking change.

I think that one has been around much longer, yeah. But i'm just going from memory. We could throw a special deprecation exception instead, right?

This comment has been minimized.

Copy link
@hub-cap

hub-cap Jan 14, 2019

Author Contributor

yea this is what the code does as is, even with it being child of ResponseException, its just currently wrapped in a ElasticsearchStatusException as well.

This comment has been minimized.

Copy link
@hub-cap

hub-cap Jan 16, 2019

Author Contributor

@javanna Id like your opinion on whether to break the current functionality, which is to always return an ElasticsearchStatusException unless its a random exception we dont know about. Ill gladly throw the WarningFailureException w/o wrapping it, just want to make sure thats the correct thing to do WRT compatibility. I think @nik9000 says he is ok with it, correct @nik9000 ?

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 16, 2019

Contributor

I'm fine throwing the WarningFailureException, unwrapped, not extending ElasticsearchStatusException.

This comment has been minimized.

Copy link
@javanna

javanna Jan 22, 2019

Member

I am fine too with rethrowing WarningFailureException as-is.

@hub-cap

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

@javanna @nik9000 Now we have a nice throw (see below) from the async code, with one caveat. The async code uses this from server which expects any exception to be either an ElasticsearchException or a RuntimeException. I decided to make WarningFailureException extend RuntimeException so it would not wrap it here.

org.elasticsearch.client.WarningFailureException: method [PUT], host [http://127.0.0.1:9200], URI [/index/type/id?timeout=1m], status line [HTTP/1.1 201 Created]
Warnings: [[types removal] Specifying types in document index requests is deprecated, use the typeless endpoints instead (/{index}/_doc/{id}, /{index}/_doc, or /{index}/_create/{id}).]
---
_index: "index"
_type: "type"
_id: "id"
_version: 1
result: "created"
_shards:
  total: 2
  successful: 1
  failed: 0
_seq_no: 0
_primary_term: 1

Id love another quick look.

@hub-cap

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/1

@javanna
Copy link
Member

left a comment

LGTM

@@ -30,7 +30,7 @@
* Exception thrown when an elasticsearch node responds to a request with a status code that indicates an error.
* Holds the response that was returned.
*/
public final class ResponseException extends IOException {
public class ResponseException extends IOException {

This comment has been minimized.

Copy link
@javanna

javanna Jan 25, 2019

Member

I think that these changes to ResponseException are no longer needed?

This comment has been minimized.

Copy link
@hub-cap

hub-cap Jan 28, 2019

Author Contributor

doh, good call!

// if the exception is not of type ElasticsearchException or RuntimeException it will be wrapped in a UncategorizedExecutionException
public class WarningFailureException extends RuntimeException {

private Response response;

This comment has been minimized.

Copy link
@javanna

javanna Jan 25, 2019

Member

make it final?

@hub-cap hub-cap merged commit 2cbc688 into elastic:master Jan 30, 2019

7 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

jasontedor added a commit to dnhatn/elasticsearch that referenced this pull request Jan 30, 2019

Merge branch 'master' into pr/37940
* master:
  Expose retention leases in shard stats (elastic#37991)
  Make primary terms fields private in index shard (elastic#38036)
  ML: Add reason field in JobTaskState (elastic#38029)
  Log flush_stats and commit_stats in testMaybeFlush
  HLRC: Fix strict setting exception handling (elastic#37247)
  Test: Enable strict deprecation on all tests (elastic#36558)
  Removes typed calls from YAML REST tests (elastic#37611)
  Switch default time format for ingest from Joda to Java for v7 (elastic#37934)
  Remove deprecated Plugin#onModule extension points (elastic#37866)
  Geo: Fix Empty Geometry Collection Handling (elastic#37978)
@romseygeek

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

I think this is causing (non-reproducible) failures in FullClusterRestartIT, specifically here:

The test is anticipating a ResponseException, but it will now get a WarningFailureException instead and so won't catch it.

hub-cap added a commit to hub-cap/elasticsearch that referenced this pull request Jan 31, 2019

HLRC: Fix strict setting exception handling
The LLRC's exception handling for strict mode was previously throwing an
exception the HLRC assumed was an error response. This is not the case
if the result is valid in strict mode, as it will return the proper
response wrapped in an exception with warnings. This commit fixes the
HLRC such that it no longer spews if it encounters a strict LLRC
response.

Closes elastic#37090
Relates elastic#37247

hub-cap added a commit that referenced this pull request Jan 31, 2019

HLRC: Fix strict setting exception handling (#38112)
The LLRC's exception handling for strict mode was previously throwing an
exception the HLRC assumed was an error response. This is not the case
if the result is valid in strict mode, as it will return the proper
response wrapped in an exception with warnings. This commit fixes the
HLRC such that it no longer spews if it encounters a strict LLRC
response.

Closes #37090
Relates #37247

dnhatn added a commit that referenced this pull request Feb 2, 2019

Adapt LLRest warning exception in FullClusterRestartIT (#38253)
We now throw a WarningFailureException instead of ResponseException if
there's any warning in a response. This change leads to the failures of
testSnapshotRestore in the BWC builds for the last two days.

Relates #37247

@colings86 colings86 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.