[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40610

benwtrent · 2019-03-28T19:11:34Z

There is a bug when serializing DatafeedConfgs with aggregations from earlier versions of <= 6.5.4 to 6.7.0.

When 6.5.4 or earlier version writes to the out stream, it writes aggs in the following way
Stream:
-> stuff before
-> boolean:True
-> parsedAggs Object
-> stuff after

When 6.7.0 reads from the 6.5.4 or earlier version stream, it reads in the following way
-> stuff before
-> boolean:True
-> boolean: <--- Starts corrupting stream
-> read ParsedAggs < --- fails due to reading the "second boolean"

There is a similar situation when 6.7.0 writes out aggregations, it writes two true boolean values to the stream before writing the parsed aggs.

I have tested upgrades from 6.6.0 -> 6.7.0 and the issue does not occur.

elasticmachine · 2019-03-28T19:11:36Z

Pinging @elastic/ml-core

benwtrent · 2019-03-28T19:33:20Z

run elasticsearch-ci/2

dimitris-athanasiou · 2019-03-29T08:58:49Z

Since 6.7.0 now writes the extra boolean, don't we need version checking to handle it differently?

droberts195 · 2019-03-29T09:44:37Z

Since 6.7.0 now writes the extra boolean, don't we need version checking to handle it differently?

6.7.0 only writes the extra boolean when talking to 6.5.x or earlier. This is wrong but there's nothing we can do about it because 6.7.0 and 6.5.x are already released so cannot be changed.

The way 6.7.x talks to 6.7.y has not been changed by this PR, so it will still be possible to have a mixed version cluster of 6.7.0 and 6.7.1.

droberts195 · 2019-03-29T09:51:07Z

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

@@ -29,7 +30,34 @@
            "job_id":"old-cluster-datafeed-job",
            "indices":["airline-data"],
            "types":["response"],
-            "scroll_size": 2000
+            "scroll_size": 2000,
+            "aggregations": {


Since the wire transport is quite different for null and non-null aggregations I think it would be good to have a second datafeed that doesn't use aggregations. I will push another test to this PR soon.

davidkyle · 2019-03-29T10:20:17Z

I pushed a commit adding aggregations to the datafeeds in the full cluster restart tests. This bug would not have appeared in a full cluster restart scenario but it is worth updating the tests.

The wire serialisation is different for null/non-null aggs, so it's worth testing both cases.

davidkyle

LGTM

droberts195 · 2019-03-29T15:23:30Z

I think Jenkins lost the elasticsearch-ci/packaging-sample request because the one related to the second commit was still queuing when I pushed the third commit - trying again.

Jenkins run elasticsearch-ci/packaging-sample

benwtrent · 2019-03-29T15:27:30Z

run elasticsearch-ci/packaging-sample

Conky5 · 2019-03-29T15:50:35Z

run elasticsearch-ci/packaging-sample

Conky5 · 2019-03-29T15:56:36Z

@elasticmachine run elasticsearch-ci/packaging-sample

…6.7.0 (elastic#40610) * Addressing stream failure and adding tests to catch such in the future * Add aggs to full cluster restart tests * Test BWC for datafeeds with and without aggs The wire serialisation is different for null/non-null aggs, so it's worth testing both cases.

…6.7.0 (#40610) (#40660) * Addressing stream failure and adding tests to catch such in the future * Add aggs to full cluster restart tests * Test BWC for datafeeds with and without aggs The wire serialisation is different for null/non-null aggs, so it's worth testing both cases.

…6.7.0 (elastic#40610) (elastic#40660) * Addressing stream failure and adding tests to catch such in the future * Add aggs to full cluster restart tests * Test BWC for datafeeds with and without aggs The wire serialisation is different for null/non-null aggs, so it's worth testing both cases.

…6.7.0 (#40656) * [ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 (#40610) * Addressing stream failure and adding tests to catch such in the future * Add aggs to full cluster restart tests * Test BWC for datafeeds with and without aggs The wire serialisation is different for null/non-null aggs, so it's worth testing both cases. * fixing bwc test, removal of _xpack url path * Fixing BWC test for datafeed

…6.7.0 (#40659) * [ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 (#40610) * Addressing stream failure and adding tests to catch such in the future * Add aggs to full cluster restart tests * Test BWC for datafeeds with and without aggs The wire serialisation is different for null/non-null aggs, so it's worth testing both cases. * Fixing bwc test, removing types * Fixing BWC test for datafeed * Update 40_ml_datafeed_crud.yml * Update build.gradle

…6.7.0 (elastic#40610) (elastic#40660) * Addressing stream failure and adding tests to catch such in the future * Add aggs to full cluster restart tests * Test BWC for datafeeds with and without aggs The wire serialisation is different for null/non-null aggs, so it's worth testing both cases.

Addressing stream failure and adding tests to catch such in the future

0300b46

benwtrent added >bug :ml Machine learning v6.7.1 labels Mar 28, 2019

droberts195 reviewed Mar 29, 2019

View reviewed changes

Add aggs to full cluster restart tests

2cdf5a6

droberts195 changed the title ~~[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.0) -> 6.7.0~~ [ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 Mar 29, 2019

Test BWC for datafeeds with and without aggs

aa804e6

The wire serialisation is different for null/non-null aggs, so it's worth testing both cases.

davidkyle approved these changes Mar 29, 2019

View reviewed changes

droberts195 added the blocker label Mar 29, 2019

benwtrent merged commit 25aa01e into elastic:6.7 Mar 29, 2019

benwtrent deleted the bugfix/ml-agg-bwc-datafeed-serialization-67 branch March 29, 2019 20:30

benwtrent mentioned this pull request Mar 29, 2019

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40656

Merged

This was referenced Mar 29, 2019

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40659

Merged

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40610

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40610

benwtrent commented Mar 28, 2019 •

edited by droberts195

elasticmachine commented Mar 28, 2019

benwtrent commented Mar 28, 2019

dimitris-athanasiou commented Mar 29, 2019

droberts195 commented Mar 29, 2019

droberts195 Mar 29, 2019

davidkyle commented Mar 29, 2019

davidkyle left a comment

droberts195 commented Mar 29, 2019

benwtrent commented Mar 29, 2019

Conky5 commented Mar 29, 2019

Conky5 commented Mar 29, 2019

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40610

[ML] Addressing bug streaming DatafeedConfig aggs from (<= 6.5.4) -> 6.7.0 #40610

Conversation

benwtrent commented Mar 28, 2019 • edited by droberts195

elasticmachine commented Mar 28, 2019

benwtrent commented Mar 28, 2019

dimitris-athanasiou commented Mar 29, 2019

droberts195 commented Mar 29, 2019

droberts195 Mar 29, 2019

Choose a reason for hiding this comment

davidkyle commented Mar 29, 2019

davidkyle left a comment

Choose a reason for hiding this comment

droberts195 commented Mar 29, 2019

benwtrent commented Mar 29, 2019

Conky5 commented Mar 29, 2019

Conky5 commented Mar 29, 2019

benwtrent commented Mar 28, 2019 •

edited by droberts195