Removed ValuesSourceRegistry.registerAny() #55747
Conversation
All ValuesSourceTypes must be registered explicitly
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
| @@ -288,4 +291,6 @@ public String value() { | |||
| return name().toLowerCase(Locale.ROOT); | |||
| } | |||
|
|
|||
| /** List containing all members of the enumeration. */ | |||
| public static List<ValuesSourceType> ALL = Arrays.asList(CoreValuesSourceType.values()); | |||
There was a problem hiding this comment.
I think ALL might be a bit confusing here. I wonder if renaming it into ALL_CORE or using Arrays.asList(CoreValuesSourceType.values()) instead of the constant will better demonstrate the actual scope here for future readers.
There was a problem hiding this comment.
To me it seemed clear that CoreValuesSourceType.ALL is about all core VST. I renamed it to CoreValuesSourceType.ALL_CORE hoping that this can't be misunderstood.
|
@not-napoleon please check commit 2c709d3 Is that ok? Did I miss anything? |
|
@elasticmachine run elasticsearch-ci/bwc |
| @@ -42,99 +41,67 @@ | |||
| */ | |||
| public class ValuesSourceRegistry { | |||
| public static class Builder { | |||
| private Map<String, List<Map.Entry<Predicate<ValuesSourceType>, AggregatorSupplier>>> aggregatorRegistry = new HashMap<>(); | |||
| private Map<String, List<Map.Entry<ValuesSourceType, AggregatorSupplier>>> aggregatorRegistry = new HashMap<>(); | |||
There was a problem hiding this comment.
This doesn't need to be a List now. We can just make it Map<String, Map<ValuesSourceType, AggregatorSupplier>>. The list was only necessary because lambdas make bad hash keys.
There was a problem hiding this comment.
Can I do it later in a separate PR? This will really mess me up if it is done in this PR.
There was a problem hiding this comment.
No problem @imotov if you are both ok with this, I will merge this PR and you can change this List to Map later.
* Backports #55747 to 7.x * All ValuesSourceTypes must be registered explicitly * Removed lambdas in ValuesSourceRegistry
Follow up to elastic#55747.
ValuesSourcesRegistryimplemented methodregisterAny()that would automatically register a default aggregator implementation for anyValuesSourceType. Although, this was a very powerful feature, it was not very flexible, as there was no way to override the default aggregator for one or moreValuesSourceTypes.The aggregations that used the
registerAny()method so far were:missing,value_countandcardinality.In this PR we remove the
registerAny()method and replace it with explicitly registering allValuesSourceTypewith their aggregator. This means that allCoreValuesSourceTypeclasses are registered explicitly with their aggregators (formissing,value_countandcardinality).For the sub-classes of
ValuesSourceTypethat live in plugins, registering their aggregator is delegated to the plugins. This allows plugins to register their own implementation of common aggregations.