[Rollup] Remove builders from HistoGroupConfig #32533

tlrx · 2018-08-01T08:40:33Z

Same motivation as #32507 but for the HistoGroupConfig configuration object.

Related to #29827

elasticmachine · 2018-08-01T08:40:34Z

Pinging @elastic/es-search-aggs

jimczi

Thanks @tlrx

jimczi · 2018-08-01T10:12:36Z

x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/config/ConfigTests.java

-        config.setFields(Collections.emptyList());
-        e = expectThrows(IllegalArgumentException.class, config::build);
-        assertThat(e.getMessage(), equalTo("Parameter [fields] must have at least one value."));
+        final String[] fields = randomBoolean() ? new String[0] : null;


should we test both all the time (no randomization) ?

jimczi · 2018-08-01T10:12:46Z

x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/config/ConfigTests.java

-        config.setInterval(-1);
-        e = expectThrows(IllegalArgumentException.class, config::build);
-        assertThat(e.getMessage(), equalTo("Parameter [interval] must be a positive long."));
+        final long interval = randomBoolean() ? 0L : -1L;


polyfractal · 2018-08-01T21:17:25Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java

@@ -141,4 +139,8 @@ public static TermsGroupConfig randomTermsGroupConfig(final Random random) {
    private static String randomField(final Random random) {
        return randomAsciiAlphanumOfLengthBetween(random, 5, 10);
    }
+
+    private static long randomInterval(final Random random) {
+        return RandomNumbers.randomLongBetween(random, 0L, Long.MAX_VALUE);


I think this needs to be randomLongBetween(random, 1L, Long.MAX_VALUE);, since the validation check makes sure the interval is a value greater than 0.

polyfractal · 2018-08-01T21:23:08Z

x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/IndexerUtilsTests.java

@@ -354,7 +355,7 @@ public void testKeyOrdering() {
        });

        GroupConfig.Builder groupConfig = ConfigTestHelpers.getGroupConfig();
-        groupConfig.setHisto(ConfigTestHelpers.getHisto().setFields(Collections.singletonList("abc")).build());
+        groupConfig.setHisto(randomHistogramGroupConfig(random()));


Hmm, the contents of the config don't actually matter for this test because the test is just looking at agg keys, but perhaps we should add a comment to that effect? Just to prevent confusion in the future? e.g. if someone adds an assertion about the document contents, they will fail because the intervals, etc will be random.

I added a comment.

polyfractal

❤️

tlrx · 2018-08-02T15:55:47Z

Thanks a lot @polyfractal and @jimczi. I merged and took care of your comments.

Related to #29827

* master: HLRC: Move commercial clients from XPackClient (#32596) Add cluster UUID to Cluster Stats API response (#32206) Security: move User to protocol project (#32367) [TEST] Test for shard failures, add debug to testProfileMatchesRegular Minor fix for javadoc (applicable for java 11). (#32573) Painless: Move Some Lookup Logic to PainlessLookup (#32565) TEST: Avoid merges in testSeqNoAndCheckpoints [Rollup] Remove builders from HistoGroupConfig (#32533) Mutes failing SQL string function tests due to #32589 fixed elements in array of produced terms (#32519) INGEST: Enable default pipelines (#32286) Remove cluster state initial customs (#32501) Mutes LicensingDocumentationIT due to #32580 [ML] Remove multiple_bucket_spans (#32496) [ML] Rename JobProvider to JobResultsProvider (#32551) Correct minor typo in explain.asciidoc for HLRC Build: Add elastic maven to repos used by BuildPlugin (#32549) Clarify the error message when a pipeline agg is used in the 'order' parameter. (#32522) Revert "[test] turn on host io cache for opensuse (#32053)" Enable packaging tests on suse boxes [ML] Improve error when no available field exists for rule scope (#32550) [ML] Improve error for functions with limited rule condition support (#32548) Painless: Clean Up PainlessField (#32525) Add @AwaitsFix for #32554 Remove broken @link in Javadoc Scripting: Conditionally use java time api in scripting (#31441) [ML] Fix thread leak when waiting for job flush (#32196) (#32541) Add AwaitsFix to failing test - see #32546 Core: Minor size reduction for AbstractComponent (#32509) SQL: Added support for string manipulating functions with more than one parameter (#32356) [DOCS] Reloadable Secure Settings (#31713) Watcher: Reenable HttpSecretsIntegrationTests#testWebhookAction test (#32456) [Rollup] Remove builders from TermsGroupConfig (#32507) Use hostname instead of IP with SPNEGO test (#32514) Switch x-pack rolling restart to new style Requests (#32339) NETWORKING: Fix Netty Leaks by upgrading to 4.1.28 (#32511) [DOCS] Small fixes in rule configuration page (#32516) Painless: Clean up PainlessMethod (#32476) Build: Remove shadowing from benchmarks (#32475) Docs: Add all JDKs to CONTRIBUTING.md Add licensing enforcement for FIPS mode (#32437) SQL: Add test for handling of partial results (#32474) Mute testFilterCacheStats [ML][DOCS] Fix typo applied_to => applies_to Scripting: Fix painless compiler loader to know about context classes (#32385)

* 6.x: [Kerberos] Use canonical host name (#32588) Cross-cluster search: preserve cluster alias in shard failures (#32608) [TEST] Allow to run in FIPS JVM (#32607) Handle AlreadyClosedException when bumping primary term [Test] Add ckb to the list of unsupported languages (#32611) SCRIPTING: Move Aggregation Scripts to their own context (#32068) (#32629) [TEST] Enhance failure message when bulk updates have failures [ML] Add ML result classes to protocol library (#32587) Suppress LicensingDocumentationIT.testPutLicense in release builds (#32613) [Rollup] Improve ID scheme for rollup documents (#32558) Mutes failing SQL string function tests due to #32589 Suppress Wildfly test in FIPS JVMs (#32543) Add cluster UUID to Cluster Stats API response (#32206) [ML] Add some ML config classes to protocol library (#32502) [TEST]Split transport verification mode none tests (#32488) [Rollup] Remove builders from DateHistogramGroupConfig (#32555) [ML] Add Detector config classes to protocol library (#32495) [Rollup] Remove builders from MetricConfig (#32536) Fix race between replica reset and primary promotion (#32442) HLRC: Move commercial clients from XPackClient (#32596) Security: move User to protocol project (#32367) Minor fix for javadoc (applicable for java 11). (#32573) Painless: Move Some Lookup Logic to PainlessLookup (#32565) Core: Minor size reduction for AbstractComponent (#32509) INGEST: Enable default pipelines (#32286) (#32591) TEST: Avoid merges in testSeqNoAndCheckpoints [Rollup] Remove builders from HistoGroupConfig (#32533) fixed elements in array of produced terms (#32519) Mutes ReindexFailureTests.searchFailure dues to #28053 Mutes LicensingDocumentationIT due to #32580 Remove the SATA controller from OpenSUSE box [ML] Rename JobProvider to JobResultsProvider (#32551)

tlrx added review v7.0.0 >refactoring :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v6.5.0 labels Aug 1, 2018

tlrx requested review from polyfractal and jimczi August 1, 2018 08:40

jimczi approved these changes Aug 1, 2018

View reviewed changes

$polyfractal$

polyfractal reviewed Aug 1, 2018

View reviewed changes

$polyfractal$

polyfractal approved these changes Aug 1, 2018

View reviewed changes

tlrx added 4 commits August 2, 2018 15:33

[Rollup] Remove builders from HistoGroupConfig

f79d18a

remove randomization

377e39a

Fix compilation error

3b14d4d

Apply feedback

a8168b3

tlrx force-pushed the remove-builders-from-rollup-histo-group-config branch from 8f327e9 to a8168b3 Compare August 2, 2018 13:33

tlrx merged commit 08e4f4b into elastic:master Aug 2, 2018

tlrx deleted the remove-builders-from-rollup-histo-group-config branch August 2, 2018 15:55

tlrx added a commit that referenced this pull request Aug 2, 2018

[Rollup] Remove builders from HistoGroupConfig (#32533)

da0fc08

Related to #29827

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

[Rollup] Remove builders from HistoGroupConfig #32533

[Rollup] Remove builders from HistoGroupConfig #32533

tlrx commented Aug 1, 2018

elasticmachine commented Aug 1, 2018

jimczi left a comment

jimczi Aug 1, 2018

tlrx Aug 1, 2018

jimczi Aug 1, 2018

tlrx Aug 1, 2018

$@polyfractal$ polyfractal Aug 1, 2018

tlrx Aug 2, 2018

$@polyfractal$ polyfractal Aug 1, 2018

tlrx Aug 2, 2018

$@polyfractal$ polyfractal left a comment

tlrx commented Aug 2, 2018

[Rollup] Remove builders from HistoGroupConfig #32533

[Rollup] Remove builders from HistoGroupConfig #32533

Conversation

tlrx commented Aug 1, 2018

elasticmachine commented Aug 1, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal left a comment

Choose a reason for hiding this comment

tlrx commented Aug 2, 2018

$@polyfractal$ polyfractal left a comment