deprecate types for watcher #37594

jakelandis · 2019-01-17T23:06:04Z

this commit adds deprecation warnings for index actions
and search actions when executed via watcher

this commit adds deprecation warnings for index actions and search actions when executed via watcher relates elastic#35190

jtibshirani

Thanks @jakelandis, this looks good to me overall from the types deprecation side. A couple comments about testing:

It would be nice to add a test for an index action that omits the type parameter completely, as I don't think this was allowed before we introduced the typeless _bulk and index APIs.
Relatedly, we've been making sure to update the REST tests to use the new typeless APIs wherever possible. I noticed that 70_put_watch_with_index_action_using_id and other tests still use a document type. It would be good to make these tests typeless (adding a skip for all versions < 7.0, since they don't support omitting the document type), and also create another one 71_..._with_types to make sure to still test the old functionality in a mixed-version setting. This PR gives a good example of how we've been updating the REST yml tests: Deprecate the document create endpoint. #36863

spinscale · 2019-01-18T10:26:57Z

as we are using the WatcherSearchTemplateRequest also for search transforms this is implicitely covered as well.

One thing to note: This will be logged with every watch execution.

spinscale

fine from the watcher side of things

jtibshirani · 2019-01-18T22:16:50Z

I'm not sure if you're looking for another review just yet, but in hopes of saving you some time:

For the typeless REST tests, I think we'll actually need to specify a skip section, because nodes before 7.0 don't accept typeless index requests. Here's an example from a previous PR: https://github.com/elastic/elasticsearch/pull/36863/files#diff-05a8566fd2d3a7a9ced64de5aac0255dR5
Because these tests now only run against 7.0, when necessary we've been creating duplicate versions that still used typed requests. These duplicate tests have the suffix _with_types.yml (for example https://github.com/elastic/elasticsearch/pull/36863/files#diff-c26046a2ed4d0bd4d4560280802d48d6R2). This lets us continue to test important features in a mixed-node cluster.

jakelandis · 2019-01-18T23:22:10Z

@jtibshirani - not quite ready for review. I have a few more tests I am stabilizing.

However, for any of the REST tests under https://github.com/elastic/elasticsearch/tree/master/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher I am pretty confident that they do not need to be bwc since they are not run in any mixed cluster environment.

I will ping you here when this is ready for re-review.

jakelandis · 2019-01-19T01:28:25Z

.../plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherUtilsTests.java

@@ -90,7 +90,7 @@ public void testResponseToData() throws Exception {

    public void testSerializeSearchRequest() throws Exception {
        String[] expectedIndices = generateRandomStringArray(5, 5, true);
-        String[] expectedTypes = generateRandomStringArray(2, 5, true);
+        String[] expectedTypes = generateRandomStringArray(2, 5, true, false);


This parameter controls if there can be an types = [] (empty array). Empty types will be treated as null, but will still trigger deprecation warning. I chose to dis-allow this in the tests since it is an odd/rare case of specifying types, but without specifying types which would make the tests more complex then they already are (3 states instead of 2 when determining if it hasTypes).

This is a very minor point, but I wonder if it would be possible to do a small refactor in WatcherSearchTemplateRequest#fromXContent so that an empty types array is not treated as type == null?

jakelandis · 2019-01-19T04:38:56Z

@jtibshirani @spinscale - would mind taking another look. I have updated the all of the testing to either be type free or deprecation aware.

jtibshirani

I am pretty confident that they do not need to be bwc since they are not run in any mixed cluster environment.

Thanks, I wasn't aware that these tests weren't run in a mixed-version set-up like the other REST yml tests. My one question here is whether you feel the old behavior (in particular supplying types in index actions) still has some test coverage? We will still be supporting it for the duration of 7.x.

One area I overlooked in my original review is documentation. For example, it would be great to update watcher/actions/index.asciidoc to no longer specify doc_type in the index action.

Lastly, I wanted to double-check that we are fine with this approach of issuing a deprecation warning on every watch execution, and also for 'get watch' requests that load deprecated watch definitions.

jtibshirani · 2019-01-21T21:31:35Z

.../plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherUtilsTests.java

@@ -90,7 +90,7 @@ public void testResponseToData() throws Exception {

    public void testSerializeSearchRequest() throws Exception {
        String[] expectedIndices = generateRandomStringArray(5, 5, true);
-        String[] expectedTypes = generateRandomStringArray(2, 5, true);
+        String[] expectedTypes = generateRandomStringArray(2, 5, true, false);


This is a very minor point, but I wonder if it would be possible to do a small refactor in WatcherSearchTemplateRequest#fromXContent so that an empty types array is not treated as type == null?

jakelandis · 2019-01-23T14:40:38Z

@jtibshirani thanks for the re-review.

I wasn't aware that these tests weren't run in a mixed-version set-up like the other REST yml tests.

Same here until I tried to explicitly test them and couldn't find a way to run them. It isn't just watcher, but all the x-pack tests in this package.

one question here is whether you feel the old behavior (in particular supplying types in index actions) still has some test coverage?

Yes, index actions are explicitly unit tested here and integration here. Search is similarly tested. While we don't have as many REST tests that cover this flow, the unit tests do a good job and the REST integration covers the paths in question.

One area I overlooked in my original review is documentation

Agree'd but would prefer to do that on a separate PR.

Lastly, I wanted to double-check that we are fine with this approach of issuing a deprecation warning on every watch execution, and also for 'get watch' requests that load deprecated watch definitions.

It will send the deprecation header, but may or may not log based on the behavior of deprecatedAndMaybeLog. For internal watch execution (ie. the scheduled watch) the header will just be discarded, there isn't a worry about flooding the deprecation logs.

This is a very minor point, but I wonder if it would be possible to do a small refactor in WatcherSearchTemplateRequest#fromXContent so that an empty types array is not treated as type == null

I believe that if, for some reason, a user sets the types to [] it should be treated as null. It is odd that we accept empty array at all since this is an optional field. I would prefer in 7.x to (logically) cast [] to no type (null), so when we remove types altogether it less of a corner case.

jtibshirani

I believe that if, for some reason, a user sets the types to [] it should be treated as null.

For context, elsewhere in the search code we always model 'no types' as an empty types array, and do not use null. So I was suggesting keeping that as the standard, as opposed to allowing the types array to become null at any point. In any case, happy to do what you find cleanest here!

Types have been deprecated and this commit removes the documentation for specifying types in the index action, and search input/transform. Relates elastic#37594 elastic#35190

Types have been deprecated and this commit removes the documentation for specifying types in the index action, and search input/transform. Relates #37594 #35190

deprecate types for watcher

46372d9

this commit adds deprecation warnings for index actions and search actions when executed via watcher relates elastic#35190

jakelandis requested a review from jtibshirani January 17, 2019 23:06

jtibshirani mentioned this pull request Jan 17, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

jakelandis added >deprecation v7.0.0 :Data Management/Watcher labels Jan 17, 2019

jakelandis requested a review from spinscale January 17, 2019 23:12

jtibshirani reviewed Jan 18, 2019

View reviewed changes

spinscale approved these changes Jan 18, 2019

View reviewed changes

jakelandis added 3 commits January 18, 2019 09:26

deprecated unit test

22936c5

remove type from rest tests

85f41db

Merge branch 'master' into watcher_types_deprecation

7c08169

jakelandis added 3 commits January 18, 2019 18:34

Additional fixes

8406ad5

remove unused import

c583508

dis-allow empty types array in tests

833a0e4

jakelandis commented Jan 19, 2019

View reviewed changes

jtibshirani reviewed Jan 21, 2019

View reviewed changes

ycombinator mentioned this pull request Jan 23, 2019

Remove types from Monitoring plugin "backend" code #37745

Merged

jtibshirani approved these changes Jan 23, 2019

View reviewed changes

Merge branch 'master' into watcher_types_deprecation

1832bc3

jakelandis merged commit 99b75a9 into elastic:master Jan 28, 2019

jakelandis mentioned this pull request Jan 30, 2019

Remove types from watcher docs #38002

Merged

jakelandis added a commit that referenced this pull request Jan 30, 2019

Remove types from watcher docs (#38002)

6a78b6a

Types have been deprecated and this commit removes the documentation for specifying types in the index action, and search input/transform. Relates #37594 #35190

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

jakelandis mentioned this pull request Mar 6, 2019

Remove the index type from internal watcher indexes #39761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate types for watcher #37594

deprecate types for watcher #37594

jakelandis commented Jan 17, 2019

jtibshirani left a comment

spinscale commented Jan 18, 2019

spinscale left a comment

jtibshirani commented Jan 18, 2019

jakelandis commented Jan 18, 2019

jakelandis Jan 19, 2019

jtibshirani Jan 21, 2019

jakelandis commented Jan 19, 2019

jtibshirani left a comment

jtibshirani Jan 21, 2019

jakelandis commented Jan 23, 2019

jtibshirani left a comment •

edited

Loading

deprecate types for watcher #37594

deprecate types for watcher #37594

Conversation

jakelandis commented Jan 17, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

spinscale commented Jan 18, 2019

spinscale left a comment

Choose a reason for hiding this comment

jtibshirani commented Jan 18, 2019

jakelandis commented Jan 18, 2019

jakelandis Jan 19, 2019

Choose a reason for hiding this comment

jtibshirani Jan 21, 2019

Choose a reason for hiding this comment

jakelandis commented Jan 19, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani Jan 21, 2019

Choose a reason for hiding this comment

jakelandis commented Jan 23, 2019

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

jtibshirani left a comment •

edited

Loading