Remove Redundant Cluster State Update Response Classes #63646
Conversation
These intermediary response types don't contain any information outside of what the shard acknowledged and acknowledged responses contain so this PR removes them. Also, it adds three constants for the three possible states of `ShardsAcknowledgedResponse`. Follow up to elastic#63335
original-brownbear
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
| /** | ||
| * {@link AckedClusterStateUpdateTask} that responds with a plain {@link AcknowledgedResponse}. | ||
| */ | ||
| public abstract class SimpleAckedStateUpdateTask extends AckedClusterStateUpdateTask<AcknowledgedResponse> { |
There was a problem hiding this comment.
Please let's not use the term "simple". Why isn't this just part of AckedClusterStateUpdateTask?
There was a problem hiding this comment.
Why isn't this just part of AckedClusterStateUpdateTask?
🤦 yea that's much better :) I pushed 996b582 , thanks!
|
|
||
| @SuppressWarnings("unchecked") | ||
| protected Response newResponse(boolean acknowledged) { | ||
| return (Response) AcknowledgedResponse.of(acknowledged); |
There was a problem hiding this comment.
I don't think this cast should be necessary. If we are saying all response classes for AckedClusterStateUpdateTask should be an AcknowledgedResponse, then why is it a generic? Should the generic be extending AcknowledgedResponse?
There was a problem hiding this comment.
I tried this but annoyingly org.elasticsearch.xpack.ilm.action.TransportRemoveIndexLifecyclePolicyAction uses a Response class that doesn't extend AcknowledgedResponse which prevents us from doing this.
At least I couldn't find a quick+dry way around the problem for now other than what I had before with subclassing AckedClusterStateUpdateTask but even that was a lot of extra code, the current version seems simplest to me because changing the response in the ILM action isn't so trivial BwC wise?
There was a problem hiding this comment.
Is that ILM class actually utilizing the "acked" part of AckedClusterStateUpdateTask then? It seems odd based on naming alone that there is nothing asked about the response?
There was a problem hiding this comment.
Actually it isn't using any of the acked mechanics so I turned it into a normal update in 926442c and then constrained the type of the response some to actually by an acknowledged response.
Still need the cast though unfortunately, we can only get around that by sub-classing for plain AcknowledgedResponse I think (this is because the listener might have a more constrained type than AcknowledgedResponse so without the cast the generic type constraints aren't met).
There was a problem hiding this comment.
I only see two classes that concretely implement AckedClusterStateUpdateTask. Yet they seem to override everything. What benefit does the generic type give us? It seems the abstract class could concretely use AcknowledgedResponse, and those two subclasses can override and refine the response type of newResponse.
There was a problem hiding this comment.
Fair point, the generic is pointless. I removed it in 8ca8862 . Still requires one annoying case in the constructor, but much nicer IMO.
We do have a few more than 2 overrides to the response type though in the anonymous implementations. Some of those look completely redundant (e.g. org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction.Response though and could be removed in a follow-up).
Let me know what you think :)
|
|
||
| private final ActionListener<Response> listener; | ||
| private final ActionListener<AcknowledgedResponse> listener; |
There was a problem hiding this comment.
We could use ? extends here as well so a cast is not necessary?
There was a problem hiding this comment.
That won't work unfortunately. If we bound by ? extends then it could be that the listener is actually a listener for some subclass of AcknowledgedResponse so that gives a compile time error ... it always seems like this should work though :)
|
Thanks Ryan! |
These intermediary response types don't contain any information outside of what the shard acknowledged and acknowledged responses contain so this PR removes them. Also, it adds three constants for the three possible states of `ShardsAcknowledgedResponse`. Follow up to #63335
These intermediary response types don't contain any information
outside of what the shard acknowledged and acknowledged responses
contain so this PR removes them.
Also, it adds three constants for the three possible states of
ShardsAcknowledgedResponse.Follow up to #63335