[ML] Add ML filter update API #31437

dimitris-athanasiou · 2018-06-19T17:08:01Z

This adds an api to allow updating a filter:

POST _xpack/ml/filters/{filter_id}/_update

The request body may have:

description: setting a new description
add_items: a list of the items to add
remove_items: a list of the items to remove

This commit also changes the PUT filter api to
error when the filter_id is already used. As
now there is an api for updating filters, the
put api should only be used to create new ones.

Also, updating a filter results into a notification
message auditing the change for every job that is
using that filter.

elasticmachine · 2018-06-19T17:08:03Z

Pinging @elastic/ml-core

davidkyle · 2018-06-21T08:27:53Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateFilterAction.java

+        private static final ObjectParser<Request, Void> PARSER = new ObjectParser<>(NAME, Request::new);
+
+        static {
+            PARSER.declareString((request, filterId) -> request.filterId = filterId, MlFilter.ID);


This looks odd that it doesn't have a setter. Can you use the ConstructingObjectParser

As discussed, we follow this pattern in a few request objects where there is a field that will also be in the URI. While it seems odd, it saves us from having to relax the null check in the constructor. At the end, it makes the java API cleaner at the cost of having that funky lambda in the parser.

davidkyle · 2018-06-21T08:30:28Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateFilterAction.java

+
+        public void setAddItems(Collection<String> addItems) {
+            ExceptionsHelper.requireNonNull(addItems, ADD_ITEMS.getPreferredName());
+            this.addItems = new TreeSet<>();


this.addItems = new TreeSet<>(addItems); and save one line of code

davidkyle · 2018-06-21T08:30:43Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateFilterAction.java

+
+        public void setRemoveItems(Collection<String> removeItems) {
+            ExceptionsHelper.requireNonNull(removeItems, REMOVE_ITEMS.getPreferredName());
+            this.removeItems = new TreeSet<>();


davidkyle · 2018-06-21T08:32:32Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateFilterAction.java

+            addItems = new TreeSet<>();
+            addItems.addAll(Arrays.asList(in.readStringArray()));
+            removeItems = new TreeSet<>();
+            removeItems.addAll(Arrays.asList(in.readStringArray()));


Same comment about saving one line of code

davidkyle · 2018-06-21T09:08:43Z

...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateFilterAction.java

+                        BytesReference docSource = getDocResponse.getSourceAsBytesRef();
+                        try (InputStream stream = docSource.streamInput();
+                             XContentParser parser =
+                                     XContentFactory.xContent(getDocResponse.getSourceAsBytes())


Is this method not deprecated? I think it should be
XContentParser parser = XContentFactory.xContent(XContentType.JSON) .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, stream))

davidkyle · 2018-06-21T09:20:21Z

...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateFilterAction.java

+            return;
+        }
+
+        String description = filter.getDescription();


String description = request.getDescription() == null ? filter.getDescription() : request.getDescription() for readability

davidkyle · 2018-06-21T09:40:30Z

x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml

+
+---
+"Test update filter":
+


nit: delete line

davidkyle · 2018-06-21T09:45:16Z

x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml

+
+---
+"Test update filter given remove item is not present":
+


nit: extra line

davidkyle · 2018-06-21T09:45:38Z

x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java

@@ -5,6 +5,7 @@
 */
 package org.elasticsearch.xpack.ml.job;

+import com.google.common.collect.Sets;


Did intellij do this?

davidkyle

LGTM

davidkyle · 2018-06-21T12:58:38Z

x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java

        JobManager jobManager = createJobManager();

        MlFilter filter = MlFilter.builder("foo_filter").setItems("a", "b").build();

-        jobManager.updateProcessOnFilterChanged(filter);
+        jobManager.notifyFilterChanged(filter, new TreeSet<>(Arrays.asList("item 1", "item 2")), new TreeSet<>(Arrays.asList("item 3")));


nit: Collections.singletonList to avoid annoying intellji warnings

This adds an api to allow updating a filter: POST _xpack/ml/filters/{filter_id}/_update The request body may have: - description: setting a new description - add_items: a list of the items to add - remove_items: a list of the items to remove This commit also changes the PUT filter api to error when the filter_id is already used. As now there is an api for updating filters, the put api should only be used to create new ones. Also, updating a filter results into a notification message auditing the change for every job that is using that filter.

* master: Add get field mappings to High Level REST API Client (#31423) [DOCS] Updates Watcher examples for code testing (#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (#31474) [DOCS] Move monitoring to docs folder (#31477) Core: Combine doExecute methods in TransportAction (#31517) IndexShard should not return null stats (#31528) fix repository update with the same settings but different type (#31458) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) Node selector per client rather than per request (#31471) Core: Combine messageRecieved methods in TransportRequestHandler (#31519) Upgrade to Lucene 7.4.0. (#31529) [ML] Add ML filter update API (#31437) Allow multiple unicast host providers (#31509) Avoid deprecation warning when running the ML datafeed extractor. (#31463) REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514) [DOCS] Fix REST tests in SQL docs [DOCS] Add code snippet testing in more ML APIs (#31339) Core: Remove ThreadPool from base TransportAction (#31492) [DOCS] Remove fixed file from build.gradle Rename createNewTranslog to fileBasedRecovery (#31508) Test: Skip assertion on windows [DOCS] Creates field and document level security overview (#30937) [DOCS] Significantly improve SQL docs [DOCS] Move migration APIs to docs (#31473) Core: Convert TransportAction.execute uses to client calls (#31487) Return transport addresses from UnicastHostsProvider (#31426) Ensure local addresses aren't null (#31440) Remove unused generic type for client execute method (#31444) Introduce http and tcp server channels (#31446)

* 6.x: Avoid sending duplicate remote failed shard requests (#31313) Add get field mappings to High Level REST API Client Relates to #27205 [DOCS] Updates Watcher examples for code testing (#31152) [DOCS] Move monitoring to docs folder (#31477) [DOCS] Fixes SQL docs in nav [DOCS] Move sql to docs IndexShard should not return null stats - empty stats or AlreadyCloseException if it's closed is better Clarify that IP range data can be specified in CIDR notation. (#31374) Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381) In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514) fix repository update with the same settings but different type Revert "AwaitsFix FullClusterRestartIT#testRecovery" Upgrade to Lucene 7.4.0. (#31529) Avoid deprecation warning when running the ML datafeed extractor. (#31463) Retry synced-flush in FullClusterRestartIT#testRecovery Allow multiple unicast host providers (#31509) [ML] Add ML filter update API (#31437) AwaitsFix FullClusterRestartIT#testRecovery Fix missing historyUUID in peer recovery when rolling upgrade 5.x to 6.3 (#31506) Remove QueryCachingPolicy#ALWAYS_CACHE (#31451) Rename createNewTranslog to fileBasedRecovery (#31508) [DOCS] Add code snippet testing in more ML APIs (#31339) [DOCS] Remove fixed file from build.gradle [DOCS] Creates field and document level security overview (#30937) Test: Skip assertion on windows [DOCS] Move migration APIs to docs (#31473) Add a known issue for upgrading from 5.x to 6.3.0 (#31501) Return transport addresses from UnicastHostsProvider (#31426) Add Delete Snapshot High Level REST API Reload secure settings for plugins (#31481) [DOCS] Fix JDBC Maven client group/artifact ID

* elastic/master: (92 commits) Reduce number of raw types warnings (elastic#31523) Migrate scripted metric aggregation scripts to ScriptContext design (elastic#30111) turn GetFieldMappingsResponse to ToXContentObject (elastic#31544) Close xcontent parsers (partial) (elastic#31513) Ingest Attachment: Upgrade Tika to 1.18 (elastic#31252) TEST: Correct the assertion arguments order (elastic#31540) Add get field mappings to High Level REST API Client (elastic#31423) [DOCS] Updates Watcher examples for code testing (elastic#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (elastic#31474) [DOCS] Move monitoring to docs folder (elastic#31477) Core: Combine doExecute methods in TransportAction (elastic#31517) IndexShard should not return null stats (elastic#31528) fix repository update with the same settings but different type (elastic#31458) Fix Mockito trying to mock IOException that isn't thrown by method (elastic#31433) (elastic#31527) Node selector per client rather than per request (elastic#31471) Core: Combine messageRecieved methods in TransportRequestHandler (elastic#31519) Upgrade to Lucene 7.4.0. (elastic#31529) [ML] Add ML filter update API (elastic#31437) Allow multiple unicast host providers (elastic#31509) ...

dimitris-athanasiou added >non-issue review v7.0.0 :ml Machine learning v6.4.0 labels Jun 19, 2018

dimitris-athanasiou force-pushed the add-ml-filter-update-api branch 2 times, most recently from 398fbab to d54c570 Compare June 20, 2018 12:39

dimitris-athanasiou requested a review from davidkyle June 20, 2018 16:01

davidkyle reviewed Jun 21, 2018

View reviewed changes

davidkyle approved these changes Jun 21, 2018

View reviewed changes

dimitris-athanasiou force-pushed the add-ml-filter-update-api branch from 73f4511 to 4686d35 Compare June 21, 2018 17:31

dimitris-athanasiou added 5 commits June 22, 2018 11:12

Address review feedback

68b3ee1

Replace deprecated uses of XContentHelper

775ea35

Use Collections.singletonList

7eeb6c8

Fix yml tests to specify content-type

05ed4a3

dimitris-athanasiou force-pushed the add-ml-filter-update-api branch from 4686d35 to 05ed4a3 Compare June 22, 2018 10:15

Fix compilation problem after rebasing on master

899ce46

dimitris-athanasiou merged commit c6cbc99 into elastic:master Jun 22, 2018

dimitris-athanasiou deleted the add-ml-filter-update-api branch June 22, 2018 14:13

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

[ML] Add ML filter update API #31437

[ML] Add ML filter update API #31437

dimitris-athanasiou commented Jun 19, 2018

elasticmachine commented Jun 19, 2018

davidkyle Jun 21, 2018

dimitris-athanasiou Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle Jun 21, 2018

davidkyle left a comment

davidkyle Jun 21, 2018


		---
		"Test update filter":

[ML] Add ML filter update API #31437

[ML] Add ML filter update API #31437

Conversation

dimitris-athanasiou commented Jun 19, 2018

elasticmachine commented Jun 19, 2018

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

davidkyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment