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] Throw an exception if an http warning is returned by the server #33598

Closed
wants to merge 1 commit into from

Conversation

lipsill
Copy link
Contributor

@lipsill lipsill commented Sep 11, 2018

Currently the high level client silently ignores any warnings returned by the server. I stumbled on this when I deprecated a url parameter on the server side, forgot to change the high level rest client and still all client tests passed.

It is difficult to argue that any warning should result in an exception as the client does not have control over the request source provided by the user. But I think that it would be nice once a deprecated url parameter is used, the integration tests to fail (similar to the rest-api tests). What about introducing a strict testmode where any warning results in an exception? This way the used parameters and code snippets in the docs will be using always the 'correct' fields and url parameters.

I am not sure if this is something ES team is interested in pursuing. Hence this PR is intended as a starting point for a discussion rather than a solution.
(a CI run of this PR will expectedly fail as the client is currently using a deprecated endpoint)

@jimczi jimczi added >test Issues or PRs that are addressing/adding tests discuss :Core/Java High Level REST Client labels Sep 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member

Awesome! We want this (see #33534), but for the low-level REST client (then we get it for both the low-level and the high-level REST client). Would you be interested in pursuing this in the low-level REST client?

@lipsill
Copy link
Contributor Author

lipsill commented Sep 11, 2018

@jasontedor thanks for the quick feedback! I did not see that there was already an issue and a decision to proceed ;)
I will give it a shot and move the logic to the low level client.

Closing in favor of #33534

@lipsill lipsill closed this Sep 11, 2018
@jasontedor
Copy link
Member

Thanks a lot @lipsill.

@lipsill lipsill deleted the hlrc_http_warnings branch September 12, 2018 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants