Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

It's pretty common to run a block of code in a try ... catch block that just passes exceptions off to a listener's onFailure method. This commit adds a small utility to encapsulate this, enabling some one-liners.

It's pretty common to run a block of code in a `try ... catch` block
that just passes exceptions off to a listener's `onFailure` method. This
commit adds a small utility to encapsulate this, enabling some
one-liners.
@DaveCTurner DaveCTurner added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. v8.7.0 labels Jan 30, 2023
@DaveCTurner DaveCTurner requested a review from kingherc January 30, 2023 08:41
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jan 30, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

} catch (Exception e) {
listener.onFailure(e);
}
}
Copy link
Contributor

@idegtiarenko idegtiarenko Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that makes sense to have it non static?

    public void completeWith(CheckedConsumer<ActionListener<T>, Exception> action) {
        try {
            action.accept(this);
        } catch (Exception e) {
            this.onFailure(e);
        }
    }

and usage:

listener.completeWith(it -> {
...
})

Though I am not sure if this is a better naming or actually simplifies the usage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we could open that debate about most of these utilities. I'll stick with the common pattern for now tho.

@DaveCTurner
Copy link
Contributor Author

Welp I think I broke the world here. I'm sure it's something obvious but I don't see what. I'll get back to this at some point.

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment that could explain the test failures

FileInfo fileInfo = fileInfos.get();
if (fileInfo != null) {
fileSnapshotter.accept(context, fileInfo);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The listener must be completed here:

Suggested change
}
}
l.onResponse(null)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha of course. ActionRunnable#run, the gift that keeps on giving.

@DaveCTurner DaveCTurner merged commit fe50f38 into elastic:main Jan 31, 2023
@DaveCTurner DaveCTurner deleted the 2023-01-30-ActionListener.run branch January 31, 2023 07:02
mark-vieira pushed a commit to mark-vieira/elasticsearch that referenced this pull request Jan 31, 2023
It's pretty common to run a block of code in a `try ... catch` block
that just passes exceptions off to a listener's `onFailure` method. This
commit adds a small utility to encapsulate this, enabling some
one-liners.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants