Transport action for ClusterFormationFailureHelper warnings#85782
Transport action for ClusterFormationFailureHelper warnings#85782andreidan wants to merge 4 commits intoelastic:masterfrom
ClusterFormationFailureHelper warnings#85782Conversation
This adds a transport action and the necessary infrastructure to make the cluster formation warning messages available remotely via a transport action.
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @andreidan, I've created a changelog YAML for you. |
|
Pinging @elastic/clients-team (Team:Clients) |
DaveCTurner
left a comment
There was a problem hiding this comment.
👍 I left some (hopefully) simplifying suggestions.
|
|
||
| import static org.elasticsearch.rest.RestRequest.Method.GET; | ||
|
|
||
| public class RestClusterFormationInfoAction extends BaseRestHandler { |
There was a problem hiding this comment.
I don't think we need to expose this at the REST layer. In practice we're only going to use it from within the cluster, i.e. as a transport action.
There was a problem hiding this comment.
Ah fair enough, I was possibly envisioning adding more information to this API but that's probably not going to happen.
| } | ||
|
|
||
| public Optional<String> getClusterFormationWarning() { | ||
| return clusterFormationFailureHelper.emittedWarning(); |
There was a problem hiding this comment.
I think we're going to need to need a structured response, e.g. to detect partial network partitions.
There was a problem hiding this comment.
I started out making tweaks to this PR for this and wound up just putting up this draft instead -- #87306. Before I put more effort into that, do you think it's appropriate to expose all of ClusterFormationState? We seem to need most of the information in it (if not all of it), but it does seem a little odd to publicly expose a private record like that.
There was a problem hiding this comment.
It's only a private record because we've never done anything with this info except log it before. Definitely makes sense to me to make it public and use it more widely.
| * @return The result future | ||
| * @see org.elasticsearch.client.internal.Requests#clusterFormationInfoRequest(String...) | ||
| */ | ||
| ActionFuture<ClusterFormationInfoResponse> clusterFormationInfo(ClusterFormationInfoRequest request); |
There was a problem hiding this comment.
I normally wouldn't bother adding dedicated methods to the client for an API that's not going to be generally useful. You can just call Client#execute directly in the few places where it's needed.
| import org.elasticsearch.action.support.nodes.NodesOperationRequestBuilder; | ||
| import org.elasticsearch.client.internal.ElasticsearchClient; | ||
|
|
||
| public class ClusterFormationInfoRequestBuilder extends NodesOperationRequestBuilder< |
There was a problem hiding this comment.
Likewise this doesn't seem necessary, you may as well just construct the request directly.
| * This will fetch the information from the requested nodes, and it'll likely be sourced from | ||
| * {@link Coordinator}. | ||
| */ | ||
| public class TransportClusterFormationInfoAction extends TransportNodesAction< |
There was a problem hiding this comment.
I just realised that this probably can't be a TransportNodesAction: we're using this action when not a proper cluster so there's nothing holding the connections to other nodes open, so we'll have to explicitly acquire/release the connection yourself. See e.g. how Coordinator#handleJoinRequest and HotThreadsLoggingLagListener#onLagDetected wrap their transport activities in a call to TransportService#connect and release the connection afterwards. TransportNodesAction won't do that, we probably need to call TransportService#sendRequest directly.
Also it won't work to use the current cluster state to resolve node IDs since there is no cluster when using this. We'll need to use the exact DiscoveryNode instances that discovery tells us about.
|
I think we can now close this one in favor of #87306. |
This adds a transport action and the necessary infrastructure to make
the cluster formation warning messages available remotely via a
transport action.
Relates to #85624