Make ValuesSourceRegistry immutable after initilization #55493

not-napoleon · 2020-04-20T20:57:45Z

This PR separates out the registration methods from the read methods on ValuesSourceRegistry, putting the former into a new builder class. The various methods called from SearchModule to register aggregators now accept a ValuesSourceRegistryBuilder, and later uses in ValuesSourceConfig &c still use the main ValuesSourceRegistry class. SearchModule, after registering all core & plugin aggregations, constructs an immutable ValuesSourceRegistry from the builder, which it then stores and hands out as needed.

Once again, we jump through some hoops in building the immutable data structure in order to use the Java 9+ immutable collections, which use less memory and have (marginally) better access performance than the older unmodifiable wrappers. (The backport will use the unmodifiable wrappers, because of java version compatibility).

elasticmachine · 2020-04-20T20:57:52Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

nik9000

Nice! I left a few small things.

nik9000 · 2020-04-21T13:10:50Z

server/src/main/java/org/elasticsearch/plugins/SearchPlugin.java

     */
-    default List<Consumer<ValuesSourceRegistry>> getBareAggregatorRegistrar() {
+    default List<Consumer<ValuesSourceRegistry.ValuesSourceRegistryBuilder>> getBareAggregatorRegistrar() {


ValuesSourceRegistry.Builder?

Rename method to extendAggregations or something? Also, now that I'm looking at it, why return a list instead of just a method? Or even just:

default void extendAggregations(ValuesSourceRegistry.Builder builder) {}

?

I know it doesn't line up with the other "return lists of stuff" methods on these classes but ValuesSourceRegistry is a different animal anyway.

nik9000 · 2020-04-21T13:15:16Z

server/src/main/java/org/elasticsearch/search/SearchModule.java


        // after aggs have been registered, see if there are any new VSTypes that need to be linked to core fields
-        registerFromPlugin(plugins, SearchPlugin::getBareAggregatorRegistrar, this::registerBareAggregatorRegistrar);
+        registerFromPlugin(plugins, SearchPlugin::getBareAggregatorRegistrar,


Maybe it isn't worth having the method named registerBareAggregatorRegistrar now.

Just use the lambda.

nik9000 · 2020-04-21T13:16:14Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java

+         *                  applied to that type.
+         * @param aggregatorSupplier An Aggregation-specific specialization of AggregatorSupplier which will construct the mapped aggregator
+         */
+        public synchronized void register(String aggregationName, Predicate<ValuesSourceType> appliesTo,


I don't think it needs to be synchronized any more because we do all plugin building in one thread.

nik9000 · 2020-04-21T13:17:25Z

test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java

@@ -18,6 +18,26 @@
 */
 package org.elasticsearch.search.aggregations;

+import static org.elasticsearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS;


I think your import order config is a bit confused here. I believe we usually put static last. Not a big deal, but it makes the diff bigger.

alx75 · 2020-04-22T21:57:36Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java

+        /**
+         * Register a ValuesSource to Aggregator mapping.
+         *
+         * Threading behavior notes: This call is both synchronized and expensive. It copies the entire existing mapping structure each


Is this still relevant ?

Conflicts: x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java

not-napoleon · 2020-04-23T13:36:20Z

@elasticmachine run elasticsearch-ci/1

nik9000

LGTM

nik9000 · 2020-04-23T13:44:59Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java

@@ -81,7 +81,8 @@ protected void innerWriteTo(StreamOutput out) {

    @Override
    protected AvgAggregatorFactory innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config,
-                                              AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException {
+                                              AggregatorFactory parent,
+                                              AggregatorFactories.Builder subFactoriesBuilder) throws IOException {


Glad you like it. Intellij stuffed in the fully qualified package name there, and I did a manual pass yesterday to trim them down to just AggregatorFactories.Builder.

nik9000 · 2020-04-23T13:46:02Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java

-     */
-    public void registerAny(String aggregationName, AggregatorSupplier aggregatorSupplier) {
-        register(aggregationName, (ignored) -> true, aggregatorSupplier);
+    // Maps Aggregation names to (ValuesSourceType, Supplier) pairs, keyed by ValuesSourceType


Javadoc instead?

WIP: this commit just resolves merge conflicts. Still need to fix compile errors Conflicts: server/src/main/java/org/elasticsearch/search/SearchModule.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/StatsAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/AnalyticsPlugin.java x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregatorFactory.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregatorTests.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HDRPreAggregatedPercentileRanksAggregatorTests.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HDRPreAggregatedPercentilesAggregatorTests.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/TDigestPreAggregatedPercentileRanksAggregatorTests.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/TDigestPreAggregatedPercentilesAggregatorTests.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/stringstats/StringStatsAggregatorTests.java x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java

…55697)

not-napoleon added 5 commits April 16, 2020 16:49

Registry builder

835cf1b

Merge branch 'master' into vs-refactor-immutable-registry

5f12673

Production design, still need to plumb through to tests

229fde7

Fix test aggregator loading

429c21d

Copy to immutable data structures at build time

bbf6315

not-napoleon added :Analytics/Aggregations Aggregations >refactoring v8.0.0 v7.8.0 labels Apr 20, 2020

not-napoleon requested a review from nik9000 April 20, 2020 20:57

nik9000 reviewed Apr 21, 2020

View reviewed changes

not-napoleon added 5 commits April 21, 2020 12:24

Rename ValuesSourceRegistryBuilder -> Builder

85f1b30

fix checkstyle

6b3cff6

Make register private and not synchronized

fe58b1f

Merge branch 'master' into vs-refactor-immutable-registry

b063ae4

Wire up new agg extention

a316467

alx75 reviewed Apr 22, 2020

View reviewed changes

not-napoleon added 2 commits April 23, 2020 09:01

Merge branch 'master' into vs-refactor-immutable-registry

d912671

Conflicts: x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java

Removed no longer relevant javadoc

946a6e4

nik9000 approved these changes Apr 23, 2020

View reviewed changes

Javadoc

05613f4

not-napoleon merged commit 85a160f into elastic:master Apr 23, 2020

not-napoleon deleted the vs-refactor-immutable-registry branch April 23, 2020 15:54

not-napoleon added the backport pending label Apr 23, 2020

not-napoleon mentioned this pull request Apr 23, 2020

[7.x] Make ValuesSourceRegistry immutable after initilization #55493 #55697

Merged

not-napoleon added a commit that referenced this pull request Apr 24, 2020

[7.x] Make ValuesSourceRegistry immutable after initilization #55493 (#…

87b4979

…55697)

not-napoleon removed the backport pending label Apr 24, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make ValuesSourceRegistry immutable after initilization #55493

Make ValuesSourceRegistry immutable after initilization #55493

Uh oh!

not-napoleon commented Apr 20, 2020

Uh oh!

elasticmachine commented Apr 20, 2020

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Apr 21, 2020

Uh oh!

nik9000 Apr 21, 2020

Uh oh!

nik9000 Apr 21, 2020

Uh oh!

nik9000 Apr 21, 2020

Uh oh!

nik9000 Apr 21, 2020

Uh oh!

nik9000 Apr 21, 2020

Uh oh!

alx75 Apr 22, 2020

Uh oh!

not-napoleon commented Apr 23, 2020

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Apr 23, 2020

Uh oh!

not-napoleon Apr 23, 2020

Uh oh!

nik9000 Apr 23, 2020

Uh oh!

not-napoleon Apr 23, 2020

Uh oh!

Uh oh!

Make ValuesSourceRegistry immutable after initilization #55493

Make ValuesSourceRegistry immutable after initilization #55493

Uh oh!

Conversation

not-napoleon commented Apr 20, 2020

Uh oh!

elasticmachine commented Apr 20, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-napoleon commented Apr 23, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!