Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check self references in metric agg after last doc collection (#33593) #34001

Merged
merged 8 commits into from
Oct 25, 2018
Merged

Check self references in metric agg after last doc collection (#33593) #34001

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0aff5a3
Check self references in metric agg after last doc collection (#33593)
cbismuth Sep 24, 2018
cb6e307
Revert 0aff5a30c5dbad9f476be14f34b81e2d1991bb0f (#33593)
cbismuth Sep 26, 2018
cc10fec
Check self refs in metric agg only once in post collection hook (#33593)
cbismuth Sep 26, 2018
1b63f10
Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…
cbismuth Oct 1, 2018
4bfa3c3
Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…
cbismuth Oct 9, 2018
572fac7
Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…
cbismuth Oct 24, 2018
671d6c6
Remove unnecessary mocking (#33593)
cbismuth Oct 24, 2018
56a3ea7
Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…
cbismuth Oct 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion ...src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ public void collect(int doc, long bucket) throws IOException {

leafMapScript.setDocument(doc);
leafMapScript.execute();
CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script");
}
};
}
Expand All @@ -103,4 +102,14 @@ public InternalAggregation buildEmptyAggregation() {
return new InternalScriptedMetric(name, null, reduceScript, pipelineAggregators(), metaData());
}

@Override
protected void doPostCollection() throws IOException {
ensureNoSelfReferencesInAggState();

super.doPostCollection();
}

void ensureNoSelfReferencesInAggState() {
CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script");
}
}
35 changes: 33 additions & 2 deletions ...est/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,13 @@
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.IndexReader;
import org.apache.lucene.index.RandomIndexWriter;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.store.Directory;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.script.MockScriptEngine;
Expand All @@ -34,9 +38,11 @@
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.AggregationBuilder;
import org.elasticsearch.search.aggregations.Aggregator;
import org.elasticsearch.search.aggregations.AggregatorTestCase;
import org.elasticsearch.search.aggregations.metrics.ScriptedMetric;
import org.elasticsearch.search.aggregations.metrics.ScriptedMetricAggregationBuilder;
import org.elasticsearch.search.aggregations.MultiBucketConsumerService;
import org.junit.After;
import org.junit.BeforeClass;

import java.io.IOException;
Expand All @@ -48,6 +54,8 @@
import java.util.function.Function;

import static java.util.Collections.singleton;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;

public class ScriptedMetricAggregatorTests extends AggregatorTestCase {

Expand Down Expand Up @@ -81,6 +89,8 @@ public class ScriptedMetricAggregatorTests extends AggregatorTestCase {

private static final Map<String, Function<Map<String, Object>, Object>> SCRIPTS = new HashMap<>();

private ScriptedMetricAggregator aggregator = null;

@BeforeClass
@SuppressWarnings("unchecked")
public static void initMockScripts() {
Expand Down Expand Up @@ -154,6 +164,15 @@ public static void initMockScripts() {
});
}

@After
public void ensureNoSelfReferencesInAggState() {
if (aggregator != null) {
verify(aggregator).ensureNoSelfReferencesInAggState();

aggregator = null;
}
}

@SuppressWarnings("unchecked")
public void testNoDocs() throws IOException {
try (Directory directory = newDirectory()) {
Expand Down Expand Up @@ -338,6 +357,18 @@ public void testSelfReferencingAggStateAfterCombine() throws IOException {
}
}

@Override
@SuppressWarnings("unchecked")
protected <A extends Aggregator> A createAggregator(Query query,
AggregationBuilder aggregationBuilder,
IndexSearcher indexSearcher,
IndexSettings indexSettings,
MultiBucketConsumerService.MultiBucketConsumer bucketConsumer,
MappedFieldType... fieldTypes) throws IOException {
aggregator = spy(super.createAggregator(query, aggregationBuilder, indexSearcher, indexSettings, bucketConsumer, fieldTypes));
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not particularly a fan of using spy() here. There are a couple of reasons for this:

  1. The class under is being wrapped by Mockito which feels a bit weird to me because we are not really testing the class we intend to test. It also has the danger of the Mockito usage being extended in future and us accidentally bypassing the actual logic in the ScriptedMetricAggregator itself
  2. We are creating a ensureNoSelfReferencesInAggState() method purely so we can do this spying which feels a bit ugly to me.

I wonder if we need to do this spying at all and instead could maybe just rely on the existing tests that check we catch when a self-reference occurs and not worry about ensuring its only called once? It feels to me like the downsides of doing this mockito check might outweigh the benefits.

@rjernst do you have any thoughts on this?

Copy link
Contributor Author

@cbismuth cbismuth Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I understand and I do agree with you @colings86.

Another idea I can suggest would be to throw an exception from the ScriptedMetricAggregator if we do ensure no self references in agg state more than once.

EDIT But adding a counter and throwing an exception also seems overwhelming to me.

I let @rjernst give us his opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @colings86. I would rely on/improve existing tests. Mocking should not be necessary here. A mock script can be added to the existing tests which creates a cycle, and ScriptedMetricAggregator can call the normal ensureNoSelfReferences method directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @rjernst, I'll remove unnecessary mocking.

return (A) aggregator;
}

/**
* We cannot use Mockito for mocking QueryShardContext in this case because
* script-related methods (e.g. QueryShardContext#getLazyExecutableScript)
Expand Down