From 6622e8a193c1f07fe2d5b0c5ca1d59b508f8ee73 Mon Sep 17 00:00:00 2001 From: iverase Date: Mon, 18 Oct 2021 15:45:34 +0200 Subject: [PATCH 1/4] Fix circuit breaker leak in MultiTerms aggregation --- x-pack/plugin/analytics/build.gradle | 2 + .../MultiTermsWithRequestBreakerIT.java | 96 +++++++++++++++++++ .../multiterms/MultiTermsAggregator.java | 7 +- 3 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugin/analytics/src/internalClusterTest/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsWithRequestBreakerIT.java diff --git a/x-pack/plugin/analytics/build.gradle b/x-pack/plugin/analytics/build.gradle index 3f1ba843af178..4318333698e75 100644 --- a/x-pack/plugin/analytics/build.gradle +++ b/x-pack/plugin/analytics/build.gradle @@ -1,4 +1,6 @@ apply plugin: 'elasticsearch.internal-es-plugin' +apply plugin: 'elasticsearch.internal-cluster-test' + esplugin { name 'x-pack-analytics' description 'Elasticsearch Expanded Pack Plugin - Analytics' diff --git a/x-pack/plugin/analytics/src/internalClusterTest/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsWithRequestBreakerIT.java b/x-pack/plugin/analytics/src/internalClusterTest/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsWithRequestBreakerIT.java new file mode 100644 index 0000000000000..d878ae93c52e7 --- /dev/null +++ b/x-pack/plugin/analytics/src/internalClusterTest/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsWithRequestBreakerIT.java @@ -0,0 +1,96 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.xpack.analytics.multiterms; + +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.ExceptionsHelper; +import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.common.breaker.CircuitBreakingException; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.search.aggregations.support.MultiValuesSourceFieldConfig; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.xpack.analytics.AnalyticsPlugin; + +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.stream.IntStream; + +/** + * test forked from CardinalityWithRequestBreakerIT + */ +public class MultiTermsWithRequestBreakerIT extends ESIntegTestCase { + + protected Collection> nodePlugins() { + return List.of(AnalyticsPlugin.class); + } + + /** + * Test that searches using multiterms aggregations returns all request breaker memory. + */ + public void testRequestBreaker() throws Exception { + final String requestBreaker = randomIntBetween(1, 10000) + "kb"; + logger.info("--> Using request breaker setting: {}", requestBreaker); + + indexRandom( + true, + IntStream.range(0, randomIntBetween(10, 1000)) + .mapToObj( + i -> client().prepareIndex("test") + .setId("id_" + i) + .setSource(Map.of("field0", randomAlphaOfLength(5), "field1", randomAlphaOfLength(5))) + ) + .toArray(IndexRequestBuilder[]::new) + ); + + client().admin() + .cluster() + .prepareUpdateSettings() + .setPersistentSettings( + Settings.builder().put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(), requestBreaker) + ) + .get(); + + try { + client().prepareSearch("test") + .addAggregation( + new MultiTermsAggregationBuilder("xxx").terms( + List.of( + new MultiValuesSourceFieldConfig.Builder().setFieldName("field0.keyword").build(), + new MultiValuesSourceFieldConfig.Builder().setFieldName("field1.keyword").build() + ) + ) + ) + .get(); + } catch (ElasticsearchException e) { + if (ExceptionsHelper.unwrap(e, CircuitBreakingException.class) == null) { + throw e; + } + } + + client().admin() + .cluster() + .prepareUpdateSettings() + .setPersistentSettings( + Settings.builder().putNull(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING.getKey()) + ) + .get(); + + // validation done by InternalTestCluster.ensureEstimatedStats() + } +} diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregator.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregator.java index b522a5efe7eb5..65f26c391612b 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregator.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregator.java @@ -19,6 +19,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.core.CheckedConsumer; +import org.elasticsearch.core.Releasables; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.DocValueFormat; @@ -110,7 +111,6 @@ protected MultiTermsAggregator( .collect(Collectors.toList()); keyConverters = values.stream().map(TermValuesSource::keyConverter).collect(Collectors.toList()); bucketOrds = BytesKeyedBucketOrds.build(context.bigArrays(), cardinality); - } private boolean subAggsNeedScore() { @@ -220,6 +220,11 @@ public void accept(Integer start) throws IOException { }; } + @Override + protected void doClose() { + Releasables.close(bucketOrds); + } + @Override public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws IOException { InternalMultiTerms.Bucket[][] topBucketsPerOrd = new InternalMultiTerms.Bucket[owningBucketOrds.length][]; From d44e1401f70a24b202c621dc898ca99638d9671c Mon Sep 17 00:00:00 2001 From: iverase Date: Mon, 18 Oct 2021 17:25:46 +0200 Subject: [PATCH 2/4] randomize preallocation in AggregatorTestCase --- .../elasticsearch/search/aggregations/AggregatorTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 6ddcf8b041fa8..5f7eaa0c1ec1b 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -471,7 +471,7 @@ protected A searchAndReduc indexSettings, query, breakerService, - builder.bytesToPreallocate(), + randomBoolean() ? 0 : builder.bytesToPreallocate(), maxBucket, fieldTypes ); From 1c38e6efa6778d535459300b81898db75f4f449d Mon Sep 17 00:00:00 2001 From: iverase Date: Mon, 18 Oct 2021 18:02:03 +0200 Subject: [PATCH 3/4] fix CategorizeTextAggregator --- .../xpack/ml/aggs/categorization/CategorizeTextAggregator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java index 16058fbdae4f2..ed35eb86bd20d 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java @@ -105,7 +105,7 @@ protected CategorizeTextAggregator( @Override protected void doClose() { super.doClose(); - Releasables.close(this.analyzer, this.bytesRefHash); + Releasables.close(this.analyzer, this.bytesRefHash, bucketOrds); } @Override From 0324e028dfa8fd45bc2366d89c6d6e443c7985fe Mon Sep 17 00:00:00 2001 From: iverase Date: Mon, 18 Oct 2021 18:32:17 +0200 Subject: [PATCH 4/4] missed an array --- .../xpack/ml/aggs/categorization/CategorizeTextAggregator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java index ed35eb86bd20d..d413fb055dbbf 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/aggs/categorization/CategorizeTextAggregator.java @@ -105,7 +105,7 @@ protected CategorizeTextAggregator( @Override protected void doClose() { super.doClose(); - Releasables.close(this.analyzer, this.bytesRefHash, bucketOrds); + Releasables.close(this.analyzer, this.bytesRefHash, this.bucketOrds, this.categorizers); } @Override