Update tests for ElasticsearchNodeCommand #129240
Conversation
The PR updates ElasticsearchNodeCommandTests so that they test the right behaviours after multi-project related changes: 1. Persisted cluster state is always written with multi-project enabled 2. The CLI tools are expected to run on the same node version, i.e. no BWC 3. Unknown (unregistered) customs can be read and write by the CLI tools without modification Resolves: ES-11328
ywangd
requested review from
a team,
DaveCTurner,
mark-vieira,
nielsbauman and
tvernum
ywangd
added
>test
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| final Metadata latestMetadata = randomMeta(); | ||
| final XContentBuilder builder = JsonXContent.contentBuilder(); | ||
| builder.startObject(); | ||
| Metadata.FORMAT.toXContent(builder, latestMetadata); |
There was a problem hiding this comment.
This is no longer correct after MP changes because it serializes metadata without multi-project=true and that is what PersistedClusterStateService does. The CLI tools are never run across versions so that the test needs a serialized state with up-to-date format (by PersistedClusterStateService).
| Metadata loadedMetadata; | ||
| try (XContentParser parser = createParser(parserConfig, JsonXContent.jsonXContent, BytesReference.bytes(builder))) { | ||
| loadedMetadata = Metadata.fromXContent(parser); | ||
| } |
There was a problem hiding this comment.
This round-trip does not seem to directly test ElasticsearchNodeCommand at all. It is replaced directly calling ElasticsearchNodeCommand#loadTermAndClusterState.
| if (preserveUnknownCustoms) { | ||
| // check that we reserialize unknown metadata correctly again | ||
| final Path tempdir = createTempDir(); | ||
| Metadata.FORMAT.write(loadedMetadata, tempdir); |
There was a problem hiding this comment.
The node CLI always perserve unknown customs as long as they are either ClusterCustom, ProjectCustom or Condition. Therefore the PR removes test variations based on the preserveUnknownCustoms boolean. The Metadata.FORMAT.write method has the problem of not using multi-project=true when serializing. It is replaced by using the PersistedClusterStateService created as part of the node CLI.
| ); | ||
| } | ||
|
|
||
| private static class TestMissingProjectCustomMetadata extends TestProjectCustomMetadata { |
There was a problem hiding this comment.
Actually simulating unknown (unregistered) customs.
There was a problem hiding this comment.
deserves a comment in the code to that effect - the "missing" is a little confusing cos they're not missing, they're right here, the point is that they're not registered. Except they are registered in some registries, just not the one that the ElasticsearchNodeCommand uses.
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, much nicer than using IndexGraveyard to stand-in for the unknown custom particularly since I think this wasn't unknown in practice.
|
|
||
| static { | ||
| Map<String, String> params = Maps.newMapWithExpectedSize(2); | ||
| Map<String, String> params = Maps.newMapWithExpectedSize(4); |
There was a problem hiding this comment.
👍 tho unrelated to the test changes
| ); | ||
| } | ||
|
|
||
| private static class TestMissingProjectCustomMetadata extends TestProjectCustomMetadata { |
There was a problem hiding this comment.
deserves a comment in the code to that effect - the "missing" is a little confusing cos they're not missing, they're right here, the point is that they're not registered. Except they are registered in some registries, just not the one that the ElasticsearchNodeCommand uses.
ywangd
added
the
auto-merge-without-approval
|
@elasticmachine update branch |
|
@elasticmachine update branch |
4 participants
The PR updates
ElasticsearchNodeCommandTestsso that they test the actual behaviours after multi-project related changes:Resolves: ES-11328