[TEST] Fix slowness of AutoDateHistogramAggregatorTests #35072

ekalgolas · 2018-10-30T06:39:38Z

Randomize test assertion and test set size instead of asserting on an
exhaustive list of dates with fixed test set size.

Also refactor common objects used to avoid recreating them, avoid date
to string conversion and reduce duplicate test code

Randomize test assertion and test set size instead of asserting on an exhaustive list of dates with fixed test set size. Also refactor common objects used to avoid recreating them, avoid date to string conversion and reduce duplicate test code

elasticmachine · 2018-10-30T09:55:07Z

Pinging @elastic/es-search-aggs

cbuescher · 2018-11-01T10:40:31Z

@ekalgolas thanks for opening the PR looks really great at first glance but since this is a larger change it will require some more detailed attention.
Let me run some initial CI checks.
@elasticmachien test this please

cbuescher · 2018-11-02T19:54:50Z

@ekalgolas there are a couple of checkstyle errors coming from AutoDateHistogramAggregatorTests (see https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/1581/console). Could you fix those so I can re-run the tests? In general, if you run ./gradlew precommit you can catch those problems locally without having to run the whole test suite (you can even reduce that to the server sub-project with ./gradlew -p server precommit

ekalgolas · 2018-11-03T02:04:38Z

Sure, thanks for the info. Will upload a patch

Randomize test assertion and test set size instead of asserting on an exhaustive list of dates with fixed test set size. Also refactor common objects used to avoid recreating them, avoid date to string conversion and reduce duplicate test code

…ests' into fix-AutoDateHistogramAggregatorTests # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

ekalgolas · 2018-11-04T19:35:41Z

Fixed now

cbuescher · 2018-11-05T11:40:49Z

@elasticmachine test this please

cbuescher

@ekalgolas great change, I quickly compared the runtime of the test on my local machine and where it took ~25sec on average before it is now down to 10sec. Sorry it took a bit, but the change set is quite large due to the many formatting changes, but I think I got the gist of it now.

I left a few short questions and will kick of running the tests in the meantime.

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher · 2018-11-07T16:50:56Z

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

+            histogram -> {
+                final List<? extends Histogram.Bucket> buckets = histogram.getBuckets();
+                final int expectedDocCount = bucketsToExpectedDocCountMap.get(randomNumberOfBuckets);
+                final int expectedSizeCeil = (seed + expectedDoc


Where did this test move to? I'm not sure but I think it has a readon to be here and I cannot find its replacement.

Removed this test as it felt kind of redundant. In this test case we first set the buckets so that data point is a key with doc count as 1, which is tested in testRandomHourInterval and testIntervalHour. Next, this sets the number of buckets to 6, a number such that keys are now 3 hour intervals, which is again tested by testRandomHourInterval. I did not see a special case being tested cases which the existing hour test cases could not test.

I agree that part of this seems to be covered by the other randomized test, the case here seems to be somehow more carfuly crafted so I assumed it might have a special purpose. Maybe @colings86 who authored the initial commit of this test can elaborate more on this? If we want to keep it we should probably add a short note about the special purpose of this test.

I'm +1 on removing this test.

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher · 2018-11-07T17:59:54Z

@elasticmachine test this please

ekalgolas · 2018-11-09T08:35:58Z

Also I see the build has failed due to "> Task :distribution:bwc:next-bugfix-snapshot:buildBwcVersion FAILED" which might be due to #28653

…ests' into fix-AutoDateHistogramAggregatorTests # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

ekalgolas · 2018-11-09T08:38:19Z

Uploaded changes for renaming seed and undoing the conditional operator

cbuescher

Hi @ekalgolas, thanks for the update. I think the PR looks good in general but I'm wondering if we can simplify the new randomized tests a bit by making the dataset size fixed to avoid a few of the more complicated calculations now needed in the test. I left a comment along those lines, would be nice if you could try this on one method and if this still keeps runtime down apply this to all the other methods as well.

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher · 2018-11-12T11:48:59Z

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

+            histogram -> {
+                final List<? extends Histogram.Bucket> buckets = histogram.getBuckets();
+                final int expectedDocCount = bucketsToExpectedDocCountMap.get(randomNumberOfBuckets);
+                final int expectedSizeCeil = (seed + expectedDoc


I agree that part of this seems to be covered by the other randomized test, the case here seems to be somehow more carfuly crafted so I assumed it might have a special purpose. Maybe @colings86 who authored the initial commit of this test can elaborate more on this? If we want to keep it we should probably add a short note about the special purpose of this test.

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

pcsanwald

I tested this code vs master; cut down test run time from 30-40s to about 10s (local machine, small sample size, etc). Nice work! There's probably some further work we can do to reduce times, but we can tweak the randomization parameters to find a balance between test coverage and execution time.

pcsanwald · 2018-11-12T21:03:18Z

...org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

+            histogram -> {
+                final List<? extends Histogram.Bucket> buckets = histogram.getBuckets();
+                final int expectedDocCount = bucketsToExpectedDocCountMap.get(randomNumberOfBuckets);
+                final int expectedSizeCeil = (seed + expectedDoc


I'm +1 on removing this test.

…ests' into fix-AutoDateHistogramAggregatorTests # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher

@ekalgolas Thanks for the last update, LGTM now. I will kick of the CI tests and merge after we get a successful run.

cbuescher · 2018-11-19T10:41:06Z

@elasticmachine test this please

cbuescher · 2018-11-19T11:32:22Z

@ekalgolas there were some unused imports creating issues with our "checkstyle" tests. I removed them with my last commit. I hope you don't mind me pushing to your branch to speed up getting a clean CI built.
@elasticmachine test this please

cbuescher · 2018-11-19T11:51:55Z

@elasticmachine test this please

cbuescher · 2018-11-19T14:21:52Z

@ekalgolas thanks a lot for working on this, I have merged to master and will also backport to 6.x. Great speed improvement for this test indeed!

Randomize test assertion and test set size instead of asserting on an exhaustive list of dates with fixed test set size. Also refactor common objects used to avoid recreating them, avoid date to string conversion and reduce duplicate test code Closes #33181

ekalgolas · 2018-11-19T21:34:20Z

Thanks for removing the unused imports @cbuescher

pcsanwald · 2018-11-27T14:13:26Z

@ekalgolas thank you for your contribution here, much appreciated!

colings86 added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v7.0.0 v6.6.0 labels Oct 30, 2018

cbuescher self-assigned this Nov 2, 2018

ekalgolas added 2 commits November 4, 2018 11:26

Merge remote-tracking branch 'origin/fix-AutoDateHistogramAggregatorT…

84b9a3f

…ests' into fix-AutoDateHistogramAggregatorTests # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher requested changes Nov 7, 2018

View reviewed changes

Merge remote-tracking branch 'origin/fix-AutoDateHistogramAggregatorT…

09eb415

…ests' into fix-AutoDateHistogramAggregatorTests # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher requested changes Nov 12, 2018

View reviewed changes

pcsanwald reviewed Nov 12, 2018

View reviewed changes

Merge remote-tracking branch 'origin/fix-AutoDateHistogramAggregatorT…

132a956

…ests' into fix-AutoDateHistogramAggregatorTests # Conflicts: # server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorTests.java

cbuescher approved these changes Nov 19, 2018

View reviewed changes

Removing unused imports

5f2425c

Merge branch 'master' into fix-AutoDateHistogramAggregatorTests

b284baa

cbuescher merged commit 450db7f into elastic:master Nov 19, 2018

ekalgolas deleted the fix-AutoDateHistogramAggregatorTests branch November 19, 2018 21:33

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

[TEST] Fix slowness of AutoDateHistogramAggregatorTests #35072

[TEST] Fix slowness of AutoDateHistogramAggregatorTests #35072

ekalgolas commented Oct 30, 2018

elasticmachine commented Oct 30, 2018

cbuescher commented Nov 1, 2018

cbuescher commented Nov 2, 2018 •

edited

Loading

ekalgolas commented Nov 3, 2018

ekalgolas commented Nov 4, 2018

cbuescher commented Nov 5, 2018

cbuescher left a comment

cbuescher Nov 7, 2018

ekalgolas Nov 9, 2018

cbuescher Nov 12, 2018

pcsanwald Nov 12, 2018

cbuescher commented Nov 7, 2018

ekalgolas commented Nov 9, 2018

ekalgolas commented Nov 9, 2018

cbuescher left a comment •

edited

Loading

cbuescher Nov 12, 2018

pcsanwald left a comment

pcsanwald Nov 12, 2018

cbuescher left a comment

cbuescher commented Nov 19, 2018

cbuescher commented Nov 19, 2018

cbuescher commented Nov 19, 2018

cbuescher commented Nov 19, 2018 •

edited

Loading

ekalgolas commented Nov 19, 2018

pcsanwald commented Nov 27, 2018

[TEST] Fix slowness of AutoDateHistogramAggregatorTests #35072

[TEST] Fix slowness of AutoDateHistogramAggregatorTests #35072

Conversation

ekalgolas commented Oct 30, 2018

elasticmachine commented Oct 30, 2018

cbuescher commented Nov 1, 2018

cbuescher commented Nov 2, 2018 • edited Loading

ekalgolas commented Nov 3, 2018

ekalgolas commented Nov 4, 2018

cbuescher commented Nov 5, 2018

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher Nov 7, 2018

Choose a reason for hiding this comment

ekalgolas Nov 9, 2018

Choose a reason for hiding this comment

cbuescher Nov 12, 2018

Choose a reason for hiding this comment

pcsanwald Nov 12, 2018

Choose a reason for hiding this comment

cbuescher commented Nov 7, 2018

ekalgolas commented Nov 9, 2018

ekalgolas commented Nov 9, 2018

cbuescher left a comment • edited Loading

Choose a reason for hiding this comment

cbuescher Nov 12, 2018

Choose a reason for hiding this comment

pcsanwald left a comment

Choose a reason for hiding this comment

pcsanwald Nov 12, 2018

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented Nov 19, 2018

cbuescher commented Nov 19, 2018

cbuescher commented Nov 19, 2018

cbuescher commented Nov 19, 2018 • edited Loading

ekalgolas commented Nov 19, 2018

pcsanwald commented Nov 27, 2018

cbuescher commented Nov 2, 2018 •

edited

Loading

cbuescher left a comment •

edited

Loading

cbuescher commented Nov 19, 2018 •

edited

Loading