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

Introduce ActionListener#empty() #39655

Conversation

DaveCTurner
Copy link
Contributor

Today we write ActionListener.wrap(() -> {}) for an empty, do-nothing
ActionListener in a number of places. This auto-formats badly. As suggested
here[1] this commit introduces ActionListener.empty() for this situation
instead.

[1] https://github.com/elastic/elasticsearch/pull/39629/files#r262021809

Today we write `ActionListener.wrap(() -> {})` for an empty, do-nothing
`ActionListener` in a number of places. This auto-formats badly. As suggested
here[1] this commit introduces `ActionListener.empty()` for this situation
instead.

[1] https://github.com/elastic/elasticsearch/pull/39629/files#r262021809
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v8.0.0 v7.2.0 labels Mar 4, 2019
@DaveCTurner DaveCTurner requested a review from andrershov March 4, 2019 14:58
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@original-brownbear
Copy link
Member

@ywelsch pinging you here since you forbid me to do exactly this the other day :)

@ywelsch
Copy link
Contributor

ywelsch commented Mar 4, 2019

@original-brownbear perhaps explain why I wanted to avoid introducing this. My reasoning is that we should avoid using empty listeners in production code as much as possible. The issue with empty listeners is that they provide very little insight if anything goes wrong. Most times, a non-empty listener that does just trace logging is better than having a noop listener. @DaveCTurner what do you think about introducing this for tests only (e.g. in test:framework)?

@DaveCTurner
Copy link
Contributor Author

Indeed, today there is only one usage of this in production code, in IndicesClusterStateService, and it seems ok there because all calls to its failure handler are preceded by a logger.warn() call.

The usages in #39629 will be similar: there we could accumulate a number of listeners all waiting for the same connection/disconnection to complete, and it makes more sense to log the failure before notifying the listeners rather than within each listener.

However I see that this could encourage trappy behaviour - I had to put some effort into being sure that we do log every failure in that PR, and there is a risk if we introduce this API that its use will become more widespread and we will face hard-to-debug failures in future. On reflection, let's not do this.

@DaveCTurner DaveCTurner closed this Mar 4, 2019
@DaveCTurner DaveCTurner deleted the 2019-03-04-empty-actionlistener branch March 4, 2019 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants