elasticmachine
commented
5 years ago Pinging @elastic/es-core-features
Closed javanna closed 5 years ago
elasticmachine
commented
5 years ago Pinging @elastic/es-core-features
hub-cap
commented
5 years ago I did some investigation here and came up with the following.
if we just use a new exception, you end up seeing sync throwing that exception and async returning the following
UncategorizedExecutionException[Failed execution
]; nested: ExecutionException[org.elasticsearch.client.DeprecationException: method [POST], host [http://localhost:9200], URI [/foo/bar?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: "foo"
_type: "bar"
_id: "rDesGmgBurilfGV8-Oll"
_version: 1
result: "created"
_shards:
total: 2
successful: 1
failed: 0
_seq_no: 0
_primary_term: 1
]; nested: DeprecationException[method [POST], host [http://localhost:9200], URI [/foo/bar?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: "foo"
_type: "bar"
_id: "rDesGmgBurilfGV8-Oll"
_version: 1
result: "created"
_shards:
total: 2
successful: 1
failed: 0
_seq_no: 0
_primary_term: 1
];I do not think this is optimal as we have the entity there as well. we can parse the entity if it comes back valid, and I would like to give that back to the user. We cannot create an exception with the generic Response Resp from the HLRC api performRequest... requests as it is Throwable.
So we could 1) do what I have done above w/ just a new exception w/o any parsing of the response entity, 2) make all of our responses extend something that we can pull out warnings and return that instead of throwing an exception (this is the opposite of strict), or 3) we can throw an exception that just has an Object that holds the entity, as well as the List<String> of warnings, which requires casting.
or hopefully 4) something i have not yet seen that makes this easier.
hub-cap
commented
5 years ago After discussing with the team, a new option was presented. the new option, which would require some rework in the LLRC, is to have the HLRC be in charge of warnings handling if you are using the HLRC. So if someone who is using the HLRC wants to set strict, they would set it in the HLRC and not in the LLRC. This would mean requiring the final value set in the LLRC to be mutable, or the HLRC constructor that takes in a LLRC will fail in this case.
The advantages are that LLRC would not throw exceptions and that HLRC can handle them without throwing this UncategorizedExcecutionException.
Disadvantage is that you can still get the LLRC from HLRC.getLowLevelClient() and it would not have the same behavior as the HLRC youve instantiated.
I will do a bit of coding on this to see if its worth investigating further because im still not 100% sold on it, and it does not really make the "throw exception but still hold the state of the response" portion of the original issue any easier.
hub-cap
commented
5 years ago after looking things over and more discussion, this still does not solve the issue of "should we be parsing the response and throwing the exception." If we want to call onFailure and throw an exception for async/sync in HLRC, then we do not parse. I dont necessarily like this, and I would propose that instead, all of our HLRC's should extend a common response which holds info like warnings. The problem with this is that it cannot be done now, as we are using response classes from server, as well as some local to the client. So I guess for now, we need to throw this and fix it properly once we separate server from HLRC.
hub-cap
commented
5 years ago ok, lots of chatting with myself and I end up just throwing the original ElasticsearchStatusException wrapped with the new exception such that it does not try to parse it and blow up. This is the best that I could do given the constraints.
When the high-level client uses a low-level client that fails on deprecation warnings, that breaks the high-level client quite badly. The low-level client throws a
ResponseExceptionwhich is caught by the high-level client which tries to parse the response body as if it was an error, although it most likely was not. Instead of signaling the deprecation warnings clearly, the high-level client ends up breaking with:The cause of the failure does contain the deprecation warnings, but it is not so obvious that they did cause the exception to be thrown. The problem here is that a ResponseException gets thrown based on a response that returned OK status code, and no additional exception message is provided to signal what the problem was. The response itself does not necessarily tell the reason of the failure. That same response would not cause any exception with strict deprecation disabled. I believe we should use a different exception for deprecation warnings.