Removed ValuesSourceRegistry.registerAny() #55747
Conversation
All ValuesSourceTypes must be registered explicitly
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
not-napoleon
left a comment
not-napoleon
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing this! Now that we don't need to support ANY, we should have a follow-up PR to get rid of the lambdas in ValuesSourceRegistry. Let me know if you want to do that, otherwise I'll get to it as soon as I can.
| } | ||
|
|
||
| /** 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 |
not-napoleon
left a comment
not-napoleon
left a comment
There was a problem hiding this comment.
We can simplify the registry even more by just using a map of maps. Other than that, looks good. Thanks for doing this!
| 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.
6 participants
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.