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

cbismuth · 2018-09-24T14:17:54Z

I've read what has been said about a post-collection hook, it should probably be done in LeafBucketCollector as it is in BucketCollector.

I've started to work on it, but before spending more time looking in which places this hook should be called, I would like to suggest the small change in this PR.

Make it more generic doesn't seem obvious to me as the second argument of the LeafBucketCollectorBase is an Object instance.

I'm not a Lucene guru, I may be totally wrong.

elasticmachine · 2018-09-25T08:14:27Z

Pinging @elastic/es-search-aggs

colings86 · 2018-09-25T10:09:07Z

I've read what has been said about a post-collection hook, it should probably be done in LeafBucketCollector as it is in BucketCollector.

Could you explain your reasoning here? I would expect that we could check for self-references by overriding the doPostCollection() method in ScriptedMetricAggregator?

Also, I think in its current form the self-reference check will not be run in the query in the search request does not match all documents since the collect method is only called for each document that matches the query but we will only call the check if we collect all documents in the index.

cbismuth · 2018-09-25T16:40:10Z

Could you explain your reasoning here? I would expect that we could check for self-references by overriding the doPostCollection() method in ScriptedMetricAggregator?

No good reason in my reasoning. I missed the o.e.s.a.AggregatorBase#doPostCollection entry point while browsing the code base.

Also, I think in its current form the self-reference check will not be run in the query in the search request does not match all documents since the collect method is only called for each document that matches the query but we will only call the check if we collect all documents in the index.

Yes, I was afraid of that, as all tests are using the o.a.l.s.MatchAllDocsQuery API and no Painless script I tried from the documentation made the debugger stop in the o.e.s.a.LeafBucketCollectorBase#collect API.

Thank you for your help @colings86, I'll update my PR. Feel free to ask me to open a new one and close this one to avoid the last noisy commit.

cbismuth · 2018-09-26T08:49:57Z

PR updated with proper hook implementation and tested with a Mockito#verify call to check CollectionUtils.ensureNoSelfReferences is called only once per metric aggregation.

colings86 · 2018-09-27T09:27:35Z

...c/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java

+                                                        IndexSettings indexSettings,
+                                                        MultiBucketConsumerService.MultiBucketConsumer bucketConsumer,
+                                                        MappedFieldType... fieldTypes) throws IOException {
+        aggregator = spy(super.createAggregator(query, aggregationBuilder, indexSearcher, indexSettings, bucketConsumer, fieldTypes));


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

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

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?

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.

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.

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

…tric_agg_exec # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java

cbismuth · 2018-10-01T07:40:21Z

I've merged master into my branch, conflicts are resolved and PR is ready for review.

…tric_agg_exec

cbismuth · 2018-10-09T11:11:20Z

Hi, the latest comment in this PR is: do we need to test with a spied ScriptedMetricAggregator to verify single call to ensureNoSelfReferences or should we rely on existing tests?

This PR has been updated with master and all tests are green, I stay available for any necessary change.

…tric_agg_exec

cbismuth · 2018-10-24T14:31:25Z

I've updated this PR to remove unnecessary mocking and rely on existing tests, PR is ready for review.

colings86

@cbismuth thanks for updating. This LGTM. I'll set off a test run

colings86 · 2018-10-24T14:34:01Z

@elasticmachine test this please

colings86 · 2018-10-25T09:52:29Z

@cbismuth the build failed but it looks unrelated to your changes. Would you be able to update your branch with the latest master and I'll kick off another CI run?

…tric_agg_exec

cbismuth · 2018-10-25T11:22:45Z

@colings86, branch is up-to-date with master, thank you 👍

colings86 · 2018-10-25T11:33:13Z

@elasticmachine test this please

colings86 · 2018-10-25T16:13:17Z

@cbismuth thanks for working on this PR. I've now merged it to master

(#34001) * Check self references in metric agg after last doc collection (#33593) * Revert 0aff5a3 (#33593) * Check self refs in metric agg only once in post collection hook (#33593) * Remove unnecessary mocking (#33593)

colings86 · 2018-10-25T16:23:34Z

I have also backported this to 6.x. The backport is still pending on 6.5 and 6.4 and I'll do those backports tomorrow

cbismuth · 2018-10-25T17:37:02Z

Great! Thank you @colings86.

(#34001) * Check self references in metric agg after last doc collection (#33593) * Revert 0aff5a3 (#33593) * Check self refs in metric agg only once in post collection hook (#33593) * Remove unnecessary mocking (#33593)

colings86 · 2018-10-26T08:14:47Z

Backports to 6.5 and 6.4 branches now complete

* master: (74 commits) XContent: Check for bad parsers (elastic#34561) Docs: Align prose with snippet (elastic#34839) document the search context is freed if the scroll is not extended (elastic#34739) Test: Lookup node versions on rest test start (elastic#34657) SQL: Return error with ORDER BY on non-grouped. (elastic#34855) Reduce channels in AbstractSimpleTransportTestCase (elastic#34863) [DOCS] Updates Elasticsearch monitoring tasks (elastic#34339) Check self references in metric agg after last doc collection (elastic#33593) (elastic#34001) [Docs] Add `indices.query.bool.max_clause_count` setting (elastic#34779) Add 6.6.0 version to master (elastic#34847) Test: ensure char[] doesn't being with prefix (elastic#34816) Remove static import from HLRC doc snippet (elastic#34834) Logging: server: clean up logging (elastic#34593) Logging: tests: clean up logging (elastic#34606) SQL: Fix edge case: `<field> IN (null)` (elastic#34802) [Test] Mute FullClusterRestartIT.testShrink() until test is fixed SQL: Introduce ODBC mode, similar to JDBC (elastic#34825) SQL: handle X-Pack or X-Pack SQL not being available in a more graceful way (elastic#34736) [Docs] Add explanation for code snippets line width (elastic#34796) CCR: Rename follow-task parameters and stats (elastic#34836) ...

#34001) * Check self references in metric agg after last doc collection (#33593) * Revert 0aff5a3 (#33593) * Check self refs in metric agg only once in post collection hook (#33593) * Remove unnecessary mocking (#33593)

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

0aff5a3

cbismuth mentioned this pull request Sep 24, 2018

Do not check self references in metric agg per document #33593

Closed

tlrx added the :Analytics/Aggregations Aggregations label Sep 25, 2018

cbismuth added 2 commits September 26, 2018 09:28

Revert 0aff5a3 (#33593)

cb6e307

Check self refs in metric agg only once in post collection hook (#33593)

cc10fec

colings86 reviewed Sep 27, 2018

View reviewed changes

Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…

1b63f10

…tric_agg_exec # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java

Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…

4bfa3c3

…tric_agg_exec

cbismuth added 2 commits October 24, 2018 15:37

Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…

572fac7

…tric_agg_exec

Remove unnecessary mocking (#33593)

671d6c6

colings86 approved these changes Oct 24, 2018

View reviewed changes

Merge branch 'master' into 33593_no_ensure_self_ref_after_scripted_me…

56a3ea7

…tric_agg_exec

colings86 merged commit 70871b5 into elastic:master Oct 25, 2018

colings86 added the backport pending label Oct 25, 2018

cbismuth deleted the 33593_no_ensure_self_ref_after_scripted_metric_agg_exec branch October 25, 2018 17:39

colings86 added v7.0.0 v6.4.0 v6.5.0 v6.6.0 v6.4.4 v6.5.1 and removed v6.4.0 v6.5.0 backport pending labels Oct 26, 2018

colings86 added v6.4.3 >bug v6.5.0 and removed v6.4.4 v6.5.1 labels Oct 31, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cbismuth commented Sep 24, 2018

elasticmachine commented Sep 25, 2018

colings86 commented Sep 25, 2018

cbismuth commented Sep 25, 2018

cbismuth commented Sep 26, 2018

colings86 Sep 27, 2018

cbismuth Sep 27, 2018 •

edited

Loading

rjernst Oct 23, 2018

cbismuth Oct 24, 2018

cbismuth commented Oct 1, 2018

cbismuth commented Oct 9, 2018

cbismuth commented Oct 24, 2018

colings86 left a comment

colings86 commented Oct 24, 2018

colings86 commented Oct 25, 2018

cbismuth commented Oct 25, 2018

colings86 commented Oct 25, 2018

colings86 commented Oct 25, 2018

colings86 commented Oct 25, 2018

cbismuth commented Oct 25, 2018

colings86 commented Oct 26, 2018

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

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

Conversation

cbismuth commented Sep 24, 2018

elasticmachine commented Sep 25, 2018

colings86 commented Sep 25, 2018

cbismuth commented Sep 25, 2018

cbismuth commented Sep 26, 2018

colings86 Sep 27, 2018

Choose a reason for hiding this comment

cbismuth Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

rjernst Oct 23, 2018

Choose a reason for hiding this comment

cbismuth Oct 24, 2018

Choose a reason for hiding this comment

cbismuth commented Oct 1, 2018

cbismuth commented Oct 9, 2018

cbismuth commented Oct 24, 2018

colings86 left a comment

Choose a reason for hiding this comment

colings86 commented Oct 24, 2018

colings86 commented Oct 25, 2018

cbismuth commented Oct 25, 2018

colings86 commented Oct 25, 2018

colings86 commented Oct 25, 2018

colings86 commented Oct 25, 2018

cbismuth commented Oct 25, 2018

colings86 commented Oct 26, 2018

cbismuth Sep 27, 2018 •

edited

Loading