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

Add a transport action to get the features of a node #110645

Closed
wants to merge 1 commit into from

Conversation

thecoop
Copy link
Member

@thecoop thecoop commented Jul 9, 2024

Whilst this is not currently used for anything, it will be used by a cluster state listener to fix node features in cluster state, in a similar way to TransportVersionsFixupListener. This is separate to get it onto serverless before we deploy anything that uses it.

Relates to #109254

@thecoop thecoop added >non-issue :Core/Infra/Core Core issues without another label labels Jul 9, 2024
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v8.16.0 labels Jul 9, 2024
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

As mentioned in Slack I think we should consider this a TransportVersion change. I mean it does change the wire protocol, older nodes won't understand this action if we send it over the wire.

I'd also rather we added this action and its usage in one step. It kinda looks reasonable to me, but it's hard to be sure until you see how it's going to be used.

I left a couple of other inline comments.

Comment on lines +248 to +255
public ActionFuture<NodesFeaturesResponse> nodesFeatures(final NodesFeaturesRequest request) {
return execute(TransportNodesFeaturesAction.TYPE, request);
}

public void nodesFeatures(final NodesFeaturesRequest request, final ActionListener<NodesFeaturesResponse> listener) {
execute(TransportNodesFeaturesAction.TYPE, request, listener);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather not add to this API, especially for an action which is so very specialized. Let's just invoke Client#execute directly as needed.

import java.io.IOException;
import java.util.List;

@UpdateForV9 // this is not needed in v9+, all applicable versions support features
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect it might be possible for a v8.last node might try and invoke this action on a v9+ node, so we'd need to preserve this until v10 even if it's unused in the v9 codebase.

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we implementing toXContent for this response? We won't be exposing it to REST clients will we?

@thecoop
Copy link
Member Author

thecoop commented Jul 10, 2024

Implemented the whole lot in #110710

@thecoop thecoop closed this Jul 10, 2024
@thecoop thecoop deleted the nodes-features-action branch July 10, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants