[ML] rewrite deprecated datafeed configs to use new date_histogram interval fields #52538

benwtrent · 2020-02-19T21:07:03Z

interval was deprecated back in 7.2.0. It was replaced with calendar_interval and fixed_interval.

This change automatically rewrites datafeed configurations that contain the old interval field. The rewrite occurs when the configuration is read from the index. It is NOT then written back to the index.

This PR also re-enables the long disabled BWC tests for datafeeds. I did my best to cover all the flakiness. Definitely want to run through multiple times locally once #52383 is fixed.

Partially addresses: #51606

…rval

elasticmachine · 2020-02-19T21:07:06Z

Pinging @elastic/ml-core (:ml)

…:benwtrent/elasticsearch into feature/ml-datafeed-date_histo-migration

benwtrent · 2020-02-20T15:09:07Z

run elasticsearch-ci/bwc

benwtrent · 2020-02-20T15:37:07Z

run elasticsearch-ci/bwc

droberts195

Looks like a good first step. I had a couple of questions.

droberts195 · 2020-02-24T13:26:02Z

...qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/40_ml_datafeed_crud.yml

-"Put job and datafeed with aggs in old cluster - deprecated interval with warning":
+"Put job and datafeed with aggs in old cluster - deprecated interval with warning before 8.0.0":
+  - skip:
+      version: "7.99.99 - "


Could it be version: "8.0.0 - " for 8.0.0 and above?

droberts195 · 2020-02-24T13:33:35Z

.../rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/40_ml_datafeed_crud.yml

@@ -23,7 +26,9 @@ setup:
 ---
 "Test old cluster datafeed with aggs":
  - skip:
-      features: "warnings"
+      features: warnings
+      version: "all"


At the moment this test is unconditionally skipped, so I wondered about deleting it completely. But I guess the reason for keeping it is that eventually it can be reenabled. You could add a comment # TODO: remove skip section from master when version is bumped to 9.0.0 here to prompt us to do that.

Sounds good!

droberts195 · 2020-02-24T13:49:11Z

.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java

-        ElasticsearchException e = expectThrows(ElasticsearchException.class,
-                () -> createDatafeedWithDateHistogram("8d"));
-
-        assertThat(e.getMessage(), containsString("When specifying a date_histogram calendar interval [8d]"));


I think this should still be a valid test. It's testing the code in

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/extractor/ExtractorUtils.java

Lines 179 to 189 in 290c8b8

if (interval.days() > 7) {

throw ExceptionsHelper.badRequestException(invalidDateHistogramCalendarIntervalMessage(calendarInterval));

}

return interval.millis();

}

private static String invalidDateHistogramCalendarIntervalMessage(String interval) {

throw ExceptionsHelper.badRequestException("When specifying a date_histogram calendar interval ["

+ interval + "], ML does not accept intervals longer than a week because of " +

"variable lengths of periods greater than a week");

}

Did this test fail after the other changes in this PR? If so that might be a sign of a problem.

Even though histograms in general can plot variable length time buckets (e.g. months), it's still reasonable for ML to prohibit such bucket spans as they would make the modelling much more complex.

added the test back and fixed it

droberts195 · 2020-02-24T14:01:47Z

...qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/40_ml_datafeed_crud.yml

+  - is_false: datafeeds.0.node
+
+---
+"Put job and datafeed with aggs in old cluster - deprecated interval with warning after 8.0.0":


I think we will run into a problem with this in the future, because at some point before 8.0.0-beta1 is released the master branch will start to treat interval as a syntax error. This test seems to be designed for the day when there is a version beyond 8.0.0 and the old cluster is 8.0.0. But when this combination of versions is being tested this test will be forced to use "fixed_interval" : "30s" instead of "interval" : "30s", so will not test the rewrite functionality. Since it's also testing other things it's worth keeping the test, but you might as well change it to say "fixed_interval" : "30s" now.

This test seems to be designed for the day when there is a version beyond 8.0.0 and the old cluster is 8.0.0.

Not exactly, the rolling upgrade tests use an "old cluster" of 8.0.0, even if the "new cluster" version is 8.0.0

This test is effectively testing the rewrite all in the same cluster version. This is actually covered when we "GET" the old datafeed in the new cluster tests.

I will adjust the PUT to be a fixed_interval and not check for warnings at all.

droberts195

LGTM

I think we need another PR to update the configs in the index at an appropriate time, but that doesn't need to be done immediately. This is a great step towards solving the problem.

[ML] rewrite deprecated datafeed aggs to use new [fixed|calendar]inte…

4f17603

…rval

benwtrent added >refactoring :ml Machine learning v8.0.0 labels Feb 19, 2020

benwtrent and others added 4 commits February 20, 2020 07:46

fixing bwc test

612b900

Update 40_ml_datafeed_crud.yml

00367ea

adjusting bwc tests for rolling upgrade from 8.0.0

c60f8c5

Merge branch 'feature/ml-datafeed-date_histo-migration' of github.com…

efe0664

…:benwtrent/elasticsearch into feature/ml-datafeed-date_histo-migration

droberts195 reviewed Feb 24, 2020

View reviewed changes

benwtrent added 2 commits February 24, 2020 09:40

fixing bwc tests + adding back large calendar interval test

5b2d978

Merge branch 'master' into feature/ml-datafeed-date_histo-migration

252297f

benwtrent requested a review from droberts195 February 24, 2020 15:00

droberts195 approved these changes Feb 24, 2020

View reviewed changes

benwtrent merged commit b49b8db into elastic:master Feb 24, 2020

benwtrent deleted the feature/ml-datafeed-date_histo-migration branch February 24, 2020 17:43

$@polyfractal$ polyfractal mentioned this pull request Nov 4, 2020

Date histogram bwc deprecation failure #42258

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] rewrite deprecated datafeed configs to use new date_histogram interval fields #52538

[ML] rewrite deprecated datafeed configs to use new date_histogram interval fields #52538

benwtrent commented Feb 19, 2020 •

edited

Loading

elasticmachine commented Feb 19, 2020

benwtrent commented Feb 20, 2020

benwtrent commented Feb 20, 2020

droberts195 left a comment

droberts195 Feb 24, 2020

benwtrent Feb 24, 2020

benwtrent Feb 24, 2020

droberts195 Feb 24, 2020

benwtrent Feb 24, 2020

droberts195 Feb 24, 2020

benwtrent Feb 24, 2020

droberts195 Feb 24, 2020

benwtrent Feb 24, 2020

benwtrent Feb 24, 2020 •

edited

Loading

droberts195 left a comment

	if (interval.days() > 7) {
	throw ExceptionsHelper.badRequestException(invalidDateHistogramCalendarIntervalMessage(calendarInterval));
	}
	return interval.millis();
	}

	private static String invalidDateHistogramCalendarIntervalMessage(String interval) {
	throw ExceptionsHelper.badRequestException("When specifying a date_histogram calendar interval ["
	+ interval + "], ML does not accept intervals longer than a week because of " +
	"variable lengths of periods greater than a week");
	}

[ML] rewrite deprecated datafeed configs to use new date_histogram interval fields #52538

[ML] rewrite deprecated datafeed configs to use new date_histogram interval fields #52538

Conversation

benwtrent commented Feb 19, 2020 • edited Loading

elasticmachine commented Feb 19, 2020

benwtrent commented Feb 20, 2020

benwtrent commented Feb 20, 2020

droberts195 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

Choose a reason for hiding this comment

benwtrent Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

droberts195 left a comment

Choose a reason for hiding this comment

benwtrent commented Feb 19, 2020 •

edited

Loading

benwtrent Feb 24, 2020 •

edited

Loading