Do common node config on bwc tests #47361
Conversation
Before this PR we always ever ran `ElasticsearchCluster.start` once, and the common node config was never done. This becomes apparent in upgrading from `6.x` to `7.x` as the new config is missing preventing the cluster from starting.
|
Pinging @elastic/es-core-infra |
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
When we can we should prefer putting this kind of logic in ElasticsearchNode vs ElasticsearchCluster.
In any case, we need to address refactoring all this stuff to try and separate configuration from node-handling logic ASAP. All this handling of version-specific edge cases is starting to compound and make things hard to follow.
| // Can only configure master nodes if we have node names defined | ||
| if (nodeNames != null) { | ||
| if (node.getVersion().onOrAfter("7.0.0")) { | ||
| node.defaultConfig.keySet().stream() |
There was a problem hiding this comment.
I'm pretty sure that, with the exception of initial_master_nodes all this can, and probably should, go in ElasticsearchNode#start().
There was a problem hiding this comment.
Technically yes, but these are settings relating to how the cluster forms that relate to each-other,
so the settings are per node, but relate to the cluster itself.
Granted we could pull other such settings up to the cluster level.
I agree on your points on refactoring but would like to complete the back-port first.
4 participants
Before this PR we always ever ran
ElasticsearchCluster.startonce, andthe common node config was never done.
This becomes apparent in upgrading from
6.xto7.xas the new configis missing preventing the cluster from starting.