-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Remove transport client from tests #42457
Conversation
This commit removes testing infrastructure for using the transport client.
Pinging @elastic/es-core-infra |
* transport client does not understand based on the fact that it only presents the node-and-transport-client-feature. | ||
*/ | ||
@ESIntegTestCase.ClusterScope(scope = TEST) | ||
public class ClusterStateIT extends ESIntegTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjernst and I discussed this. This test is testing functionality that is currently only used for the transport client as far as I can see. This is based on the following code:
elasticsearch/server/src/main/java/org/elasticsearch/cluster/ClusterState.java
Lines 133 to 136 in a15f1ee
if (custom.getRequiredFeature().isPresent()) { | |
final String requiredFeature = custom.getRequiredFeature().get(); | |
// if it is a transport client we are lenient yet for a connected node it must have the required feature | |
return out.hasFeature(requiredFeature) || out.hasFeature(TransportClient.TRANSPORT_CLIENT_FEATURE) == false; |
At this point in time, the removal of the transport client leaves us with the NodeClient but since the NodeClient is a node, the node must have the same plugins/features that the other nodes have. Given this, I do not see a way for this test to continue to be valid without the transport client. In the future, we may decide to use the feature aware logic for remote cluster connections if we need to serialize the cluster state but this is not in place today which is why I think removal of this test is ok.
@jasontedor is this ok with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me. Thanks for checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit removes testing infrastructure for using the transport client.
This commit removes testing infrastructure for using the transport
client.