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

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

Closed
jonaslan opened this issue Sep 11, 2018 · 8 comments
Closed

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

jonaslan opened this issue Sep 11, 2018 · 8 comments
Labels
:Analytics/Aggregations Aggregations help wanted adoptme

Comments

@jonaslan
Copy link

#31044 introduced ensureNoSelfReference on the _agg object in scripted metric aggregations.
It would be nice if there was a switch to disable the it. Upgrading to 6.4 caused something around 20x performance degradation for us. And downgrading was no fun.
@jimczi jimczi added the :Analytics/Aggregations Aggregations label Sep 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jimczi jimczi added the discuss label Sep 11, 2018
@jpountz
Copy link
Contributor

jpountz commented Sep 12, 2018

I agree #31044 was not good, it should not check for self references in the aggregation state after every collected document, only once all documents are collected.

@rjernst
Copy link
Member

rjernst commented Sep 13, 2018

We discussed in FixItThursday and believe Adrien's idea should work. @polyfractal suggested doing this check in a post collection hook. I'm marking this as adoptme and updating the title accordingly.

@rjernst rjernst changed the title Flag to disable ensureNoSelfReference Do not check self references in metric agg per document Sep 13, 2018
@rjernst rjernst added help wanted adoptme and removed discuss labels Sep 13, 2018
@cbismuth
Copy link
Contributor

I'm having a look at this one.

@cbismuth
Copy link
Contributor

I've opened PR #34001, could you please have a look on it? Thank you.

colings86 pushed a commit that referenced this issue Oct 25, 2018
@cbismuth @colings86
Check self references in metric agg after last doc collection (#33593) (
70871b5 
#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 pushed a commit that referenced this issue Oct 25, 2018
@cbismuth @colings86
Check self references in metric agg after last doc collection (#33593)
7afa787 
(#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 pushed a commit that referenced this issue Oct 26, 2018
@cbismuth @colings86
Check self references in metric agg after last doc collection (#33593)
56f9ee5 
(#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 pushed a commit that referenced this issue Oct 26, 2018
@cbismuth @colings86
Check self references in metric agg after last doc collection (#33593)
464e4ab 
(#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)
jasontedor added a commit to jasontedor/elasticsearch that referenced this issue Oct 26, 2018
@jasontedor
Merge branch 'master' into ccr-docs
843ae8f 
* 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)
  ...
kcm pushed a commit that referenced this issue Oct 30, 2018
@cbismuth @kcm
Check self references in metric agg after last doc collection (#33593) (
7f01236 
#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)
@cbismuth
Copy link
Contributor

I think we can close this one as #34001 has been merged.

@polyfractal
Copy link
Contributor

Ah yep, you're right. Thanks for the ping, and the PR fix @cbismuth! :)

@cbismuth
Copy link
Contributor

You're welcome 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations help wanted adoptme
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@rjernst @jpountz @polyfractal @jonaslan @elasticmachine @jimczi @cbismuth