Cleanup more ActionListener Delegation Spots

[original-brownbear]

Cleaning up some of the remaining spots where the short-wrapper methods
could be used.

[elasticmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[rjernst]

This PR looks fine, but I have a general comment about it and recent PRs around delegation.

The concept of delegation is good here. We often have listeners that need to be wrapped to append some response or failure logic. The problem I see is with the multitude of options now available. Looking at ActionListener now, there are quite a few abstract classes for dealing with delegation. Several examples in this PR use ActionListener.Delegating. That seems to be so that the delegate is a member that is available for use within onResponse. But I find this confusing because it's difficult to understand when one class or lambda method should be used over another.

I guess my general question is where is the end goal here? While the raw lines of code have been reduced, IMO the complexity has grown considerably in understanding which utilities are available for composing action listeners.

[original-brownbear]

Thanks for taking a look + sorry for the delay here @rjernst I completely missed your review :(

I guess my general question is where is the end goal here? While the raw lines of code have been reduced, IMO the complexity has grown considerably in understanding which utilities are available for composing action listeners.

The goal here isn't just in reducing LoC and cosmetics. It's mostly to get a tighter handler on what callbacks can throw where via assertions that were recently added like #69420 and to force certain patterns on these listeners. We had a number of bugs from not having these assertions where completely unexpected failures in these listeners would just be logged somewhere (sometimes even at debug) while also causing a memory leak.

But I find this confusing because it's difficult to understand when one class or lambda method should be used over another.

That's a fair criticism. I guess I mostly go with the lambda version so long as there's not need to reference this on the listener. But admittedly in a few spots I broke with that rule when the body of the handling method was very large to make things easier to read (IMO).

The problem I see is with the multitude of options now available.

++ I think ideally we should try and move to a world where things with clearly defined behavior like Delegating survive, while things like org.elasticsearch.action.ActionListener#wrap(org.elasticsearch.common.CheckedConsumer<Response,? extends java.lang.Exception>, java.util.function.Consumer<java.lang.Exception>) that continually cause trouble via double invocations (onResponse can call onFailure causing runAfter etc. to behave strangely and such) go away step-by-step by making them stricter in regards to exception handling like we did in e.g. #50886.

[rjernst]

Thanks for the explanation @original-brownbear. The vision and this PR LGTM.

[original-brownbear]

@elasticmachine update branch

[original-brownbear]

Thanks Ryan!