Skip to content

Conversation

@williamrandolph
Copy link
Contributor

This is a follow-up to #99307, adjusting convenience methods that used to take TransportVersion arguments to account for MappingsVersion maps.

@williamrandolph williamrandolph added >non-issue :Core/Infra/Core Core issues without another label labels Sep 13, 2023
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 13, 2023
@williamrandolph williamrandolph removed the needs:triage Requires assignment of a team area label label Sep 13, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 13, 2023
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM with one comment

public Builder putCompatibilityVersions(
String nodeId,
TransportVersion transportVersion,
Map<String, SystemIndexDescriptor.MappingsVersion> systemIndexMappingsVersions
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: it seems to me that this method is always called with an empty map. Maybe we could remove the parameter and give it a clearer name, like putCompatibilityVersionsWithNoSystemIndexMapping (I know, it's long, maybe there is something better). But something that makes the caller aware they are skipping something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API I'm aiming for here is:

  1. a method that puts a CompatibilityVersions object to the map, keyed to node ID
  2. a method that sets the entire map of CompatibilityVersions for the builder
  3. a convenience method that takes all the kinds of versions and constructs a CompatibilityVersions object. As we add new kinds of versions, we'll add arguments to this method.

Since "CompatibilityVersions" is already plural, it's a little awkward to name the method that takes the whole map. I think "nodeIdToCompatibityVersionsMap" might be a more informative name.

But I think I want to keep the three-arg compatibility versions method as-is. Eventually we'll add more things to CompatibilityVersions, and I don't want to add methods to handle subsets of available types of versions.

return compatibilityVersionsMap(versions);
}

public Builder compatibilityVersionsMap(Map<String, CompatibilityVersions> versions) {
Copy link
Member

@thecoop thecoop Sep 14, 2023

Choose a reason for hiding this comment

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

Is there a name a bit more descriptive we could use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about nodeIdsToCompatibilityVersions?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me!

@williamrandolph williamrandolph added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 18, 2023
@williamrandolph
Copy link
Contributor Author

@elasticsearchmachine please run elasticsearch-ci/part-1

@elasticsearchmachine elasticsearchmachine merged commit 1f3126b into elastic:main Sep 19, 2023
@williamrandolph williamrandolph deleted the sysindex/add-mappings-versions-to-cluster-state-builder branch September 19, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants