[ML-Dataframe] add basic configuration #33813

hendrikmuhs · 2018-09-18T12:56:33Z

FEATURE BRANCH PR

add basic configuration for feature index builder job, reads the source, destination index, aggregation source and aggregation config

Note:

The AggregationConfig awaits a fix upstream, see #33942

elasticmachine · 2018-09-18T12:56:41Z

Pinging @elastic/ml-core

droberts195 · 2018-09-25T12:56:19Z

...ticsearch/xpack/ml/featureindexbuilder/action/TransportStopFeatureIndexBuilderJobAction.java

        if (taskOperationFailures.isEmpty() == false) {
-            throw org.elasticsearch.ExceptionsHelper
-                    .convertToElastic(taskOperationFailures.get(0).getCause());
+            throw org.elasticsearch.ExceptionsHelper.convertToElastic(taskOperationFailures.get(0).getCause());


You could import org.elasticsearch.ExceptionsHelper and then you wouldn't need the org.elasticsearch. here or 2 lines below.

droberts195 · 2018-09-25T13:06:12Z

...in/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/FeatureIndexBuilderJobConfig.java

+    private final SourceConfig sourceConfig;
+    private final AggregationConfig aggregationConfig;

    private static final ConstructingObjectParser<FeatureIndexBuilderJobConfig, String> PARSER = new ConstructingObjectParser<>(NAME, false,


Experience has shown that for stored configs it's best to have both a lenient parser that ignores unknown fields and a strict parser that throws an error on encountering an unknown field. See for example AnalysisConfig.LENIENT_PARSER and AnalysisConfig.STRICT_PARSER. Then use the strict parser when parsing user-supplied requests and the lenient parser when parsing configs retrieved from storage. That way things don't completely break in a mixed version cluster when the newer version has added an extra field to the config.

made a note int the meta issue (#33600), as we will highly likely move out of the cluster state (#33952) the config needs a full revisit

droberts195 · 2018-09-25T13:12:27Z

...x-builder/src/main/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/SourceConfig.java

+        this.sources = new ArrayList<>(num);
+        for (int i = 0; i < num; i++) {
+            CompositeValuesSourceBuilder<?> builder = CompositeValuesSourceParserHelper.readFrom(in);
+            getSources().add(builder);


It would be nice if this class was immutable, and this line shows why it isn't currently. This is how to make it immutable:

List<CompositeValuesSourceBuilder<?>> sources = new ArrayList<>(num); for (int i = 0; i < num; i++) { CompositeValuesSourceBuilder<?> builder = CompositeValuesSourceParserHelper.readFrom(in); sources.add(builder); } this.sources = Collections.unmodifableList(sources);

droberts195 · 2018-09-25T13:13:21Z

...x-builder/src/main/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/SourceConfig.java

+    }
+
+    public SourceConfig(List<CompositeValuesSourceBuilder<?>> sources) {
+        this.sources = sources;


For immutability:

this.sources = Collections.unmodifiableList(new ArrayList<>(sources));

droberts195 · 2018-09-25T13:20:26Z

...csearch/xpack/ml/featureindexbuilder/job/AbstractSerializingFeatureIndexBuilderTestCase.java

+
+    @Before
+    public void setUp() throws Exception {
+        super.setUp();


It's unusual to override the base class @Before method and then call the base class version first. JUnit will by default call all the base class @Before methods before the derived class @Before methods, so it would be more usual to just give the derived class @Before method a different name and let JUnit call the two methods in the standard order, avoiding the need to call super.setUp() here.

droberts195 · 2018-09-25T13:22:32Z

...src/test/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/AggregationConfigTests.java

+    public static AggregationConfig randonAggregationConfig() {
+        AggregatorFactories.Builder builder = new AggregatorFactories.Builder();
+
+        for (int i = 1; i < randomIntBetween(1, 20); ++i) {


Is it valid for the number of aggregator factories to be 0? If not maybe the AggregationConfig class itself should validate that it has at least 1?

good catch, should be for (int i = 0 ...

Nevertheless, AggregationConfig misses validation, will be part of an upcoming PR.

droberts195 · 2018-09-25T13:24:47Z

...lder/src/test/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/SourceConfigTests.java

+public class SourceConfigTests extends AbstractSerializingFeatureIndexBuilderTestCase<SourceConfig> {
+
+    public static SourceConfig randomSourceConfig() {
+        int numSources = randomIntBetween(1, 10);


Is it valid for the number of sources to be 0? If not maybe the SourceConfig class itself should validate that it has at least 1?

the for-loop below counts from 0, so there will be at least 1 source for this test.

SourceConfig lacks validation at the moment, to be addressed in an upcoming PR.

droberts195 · 2018-09-25T13:25:40Z

...src/test/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/AggregationConfigTests.java

+@AwaitsFix(bugUrl="https://github.com/elastic/elasticsearch/issues/33942")
+public class AggregationConfigTests extends AbstractSerializingFeatureIndexBuilderTestCase<AggregationConfig> {
+
+    public static AggregationConfig randonAggregationConfig() {


typo: randon -> random

droberts195 · 2018-09-25T13:29:07Z

...in/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/FeatureIndexBuilderJobConfig.java

+        indexPattern = in.readString();
+        destinationIndex = in.readString();
+        sourceConfig = in.readOptionalWriteable(SourceConfig::new);
+        aggregationConfig = in.readOptionalWriteable(AggregationConfig::new);


It seems like there are unnecessarily many levels where null is allowed. You're allowing aggregationConfig to be null here, but also in AggregationConfig aggregatorFactoryBuilder is allowed to be null. I think at most one of these possibilities should be allowed.

fixed in AggregationConfig - not aiming for at the moment but in future I see the possibility of this builder to not use aggregations.

droberts195 · 2018-09-25T13:30:51Z

...lder/src/main/java/org/elasticsearch/xpack/ml/featureindexbuilder/job/AggregationConfig.java

+    }
+
+    AggregationConfig(final StreamInput in) throws IOException {
+        aggregatorFactoryBuilder = in.readOptionalWriteable(AggregatorFactories.Builder::new);


It seems like there are unnecessarily many levels where null is allowed. You're allowing aggregatorFactoryBuilder to be null here, but also in FeatureIndexBuilderJobConfig aggregationConfig is allowed to be null. I think at most one of these possibilities should be allowed.

hendrikmuhs · 2018-09-25T15:05:49Z

@droberts195 Thanks for the review, addressed most of your comments.

For the rest I am planning to tackle them in separate PR's - too many open tasks at the moment.

droberts195

OK cool, since it's only going to a feature branch I'm happy to leave everything else for future PRs

hendrikmuhs added >feature WIP v7.0.0 :ml Machine learning labels Sep 18, 2018

hendrikmuhs changed the title ~~[ML-FIB] add basic configuration~~ [ML-Dataframe] add basic configuration Sep 19, 2018

Hendrik Muhs added 2 commits September 25, 2018 09:52

replace mocked configuration with one from job config

0e816a5

disable AggregationConfig due to upstream bug

c17d138

hendrikmuhs force-pushed the feature/fib-basic-configuration branch from 2a6d7ee to c17d138 Compare September 25, 2018 09:14

Hendrik Muhs added 2 commits September 25, 2018 14:08

add tests for SourceConfig

b1efc7b

cleanup SourceConfig

7dc55e8

droberts195 reviewed Sep 25, 2018

View reviewed changes

address review comments

446d445

hendrikmuhs removed the WIP label Sep 25, 2018

droberts195 approved these changes Sep 25, 2018

View reviewed changes

hendrikmuhs merged commit 28e426a into elastic:feature/fib Sep 25, 2018

hendrikmuhs deleted the feature/fib-basic-configuration branch October 18, 2018 08:29

hendrikmuhs removed v7.0.0 labels Dec 19, 2018

[ML-Dataframe] add basic configuration #33813

[ML-Dataframe] add basic configuration #33813

Uh oh!

Conversation

hendrikmuhs commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hendrikmuhs commented Sep 18, 2018 •

edited

Loading

hendrikmuhs commented Sep 25, 2018 •

edited

Loading