Skip to content

Swallow fatal log when ignore is specified #415

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

Closed
wants to merge 1 commit into from

Conversation

aeroastro
Copy link
Contributor

@aeroastro aeroastro commented Mar 13, 2017

The primary objective of this PR is to swallow fatal log in addition to exception, which has been supported since 6ae36e4.

In my humble opinion, when HTTP status code is explicitly specified with ignore option, receiving the status code is not fatal. For example, [404] fatal log invoked by checking document existence by HEAD request is not so informative nor required, I think.

After this PR is merged, hopefully, I also would like to replace Elasticsearch::API::Utils.__rescue_from_not_found with ignore option of perform_request, which is simpler and more efficient way with better maintainability.

@karmi karmi closed this in 9848c97 Mar 17, 2017
karmi added a commit that referenced this pull request Mar 17, 2017
This patch builds upon 9848c97 and fixes the double logging of failed responses.

Related: #415
karmi pushed a commit that referenced this pull request Mar 17, 2017
The primary objective of this PR is to swallow fatal log in addition to exception, which has been supported since 6ae36e4.

In my humble opinion, when HTTP status code is explicitly specified with ignore option, receiving the status code is not fatal. For example, [404] fatal log invoked by checking document existence by HEAD request is not so informative nor required, I think.

Closes #415
karmi added a commit that referenced this pull request Mar 17, 2017
This patch builds upon 9848c97 and fixes the double logging of failed responses.

Related: #415
@karmi
Copy link
Contributor

karmi commented Mar 17, 2017

Hi, thanks for the patch! It's indeed better to swallow the exception when the ignore parameter matches. I've noticed that the patch would cause "double logging" of the request/response, so I've fixed that in 8f6bb49.

Regarding replacing Elasticsearch::API::Utils.__rescue_from_not_found with ignore, here's the problem: I've deliberately designed the libraries as independent, so elasticsearch-transport handles only the connection stuff (HTTP, reloading connections, retries, etc) and elasticsearch-api handles only the Elasticsearch API related things (adding the Ruby methods for APIs, composing URLs, etc).

The reason is not academic -- I want the "API" to be independent on the client implementation in the elasticsearch-transport library, because people might want to use the Ruby API methods with a completely different client, possibly of their own. The only contract right now is the perform_request(method, path, params={}, body=nil) method, and adding the Elasticsearch-specific ignore options to the params feels to me to be cumbersome, weird, and brittle.

@aeroastro
Copy link
Contributor Author

Sorry for the late response, and thank you so much for providing detailed review and making this patch available.

I understand the designed independence of elasticsearch-api from client implementation like elasticsearch-transport, and now I also think it is not a good idea to replace Elasticsearch::API::Utils.__rescue_from_not_found with ignore option.

After reading some codes in elasticsearch-api libraries in light of independence, I come up with another idea to improve independence. It would be very helpful if you could take a look at a PR I am going to send later.

@aeroastro aeroastro deleted the feature/ignore-log branch May 15, 2017 13:39
@karmi
Copy link
Contributor

karmi commented May 15, 2017

@aeroastro, thanks for the understanding, please ping me on the new PR if I slip. I'm gonna be offline for the better part of the week, so I'll try to keep it in mind when I'll get to reviewing PRs next week.

@aeroastro aeroastro restored the feature/ignore-log branch May 22, 2017 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants