From f76f95b8132f591028296e9912f4b64f4d7a0cbf Mon Sep 17 00:00:00 2001 From: David Kyle Date: Tue, 22 May 2018 09:25:14 +0100 Subject: [PATCH] [ML] Filter undefined job groups from update calendar actions (#30757) The UI creates job groups in calendars ad hoc to ease calendar creation these must be filtered from the jobs list before applying updates. --- .../TransportUpdateCalendarJobAction.java | 13 +- .../xpack/ml/job/JobManager.java | 8 +- .../xpack/ml/job/persistence/JobProvider.java | 2 +- .../xpack/ml/integration/JobProviderIT.java | 2 +- .../rest-api-spec/test/ml/calendar_crud.yml | 120 +++++++++++++++--- 5 files changed, 115 insertions(+), 30 deletions(-) diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java index 0524cb28a0c..12e6fb62fd7 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java @@ -28,7 +28,6 @@ import java.util.stream.Collectors; public class TransportUpdateCalendarJobAction extends HandledTransportAction { - private final ClusterService clusterService; private final JobProvider jobProvider; private final JobManager jobManager; @@ -36,27 +35,21 @@ public class TransportUpdateCalendarJobAction extends HandledTransportAction listener) { - ClusterState clusterState = clusterService.state(); - final MlMetadata mlMetadata = MlMetadata.getMlMetadata(clusterState); - Set jobIdsToAdd = Strings.tokenizeByCommaToSet(request.getJobIdsToAddExpression()); Set jobIdsToRemove = Strings.tokenizeByCommaToSet(request.getJobIdsToRemoveExpression()); - jobProvider.updateCalendar(request.getCalendarId(), jobIdsToAdd, jobIdsToRemove, mlMetadata, + jobProvider.updateCalendar(request.getCalendarId(), jobIdsToAdd, jobIdsToRemove, c -> { - List existingJobsOrGroups = - c.getJobIds().stream().filter(mlMetadata::isGroupOrJob).collect(Collectors.toList()); - jobManager.updateProcessOnCalendarChanged(existingJobsOrGroups); + jobManager.updateProcessOnCalendarChanged(c.getJobIds()); listener.onResponse(new PutCalendarAction.Response(c)); }, listener::onFailure); } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java index 2d67e64ec60..3694e41a8b8 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java @@ -67,6 +67,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; /** * Allows interactions with jobs. The managed interactions include: @@ -420,8 +421,13 @@ public class JobManager extends AbstractComponent { public void updateProcessOnCalendarChanged(List calendarJobIds) { ClusterState clusterState = clusterService.state(); + MlMetadata mlMetadata = MlMetadata.getMlMetadata(clusterState); + + List existingJobsOrGroups = + calendarJobIds.stream().filter(mlMetadata::isGroupOrJob).collect(Collectors.toList()); + Set expandedJobIds = new HashSet<>(); - calendarJobIds.forEach(jobId -> expandedJobIds.addAll(expandJobIds(jobId, true, clusterState))); + existingJobsOrGroups.forEach(jobId -> expandedJobIds.addAll(expandJobIds(jobId, true, clusterState))); for (String jobId : expandedJobIds) { if (isJobOpen(clusterState, jobId)) { updateJobProcessNotifier.submitJobUpdate(UpdateParams.scheduledEventsUpdate(jobId), ActionListener.wrap( diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java index d7b10fb622b..8014bacf1e0 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java @@ -1114,7 +1114,7 @@ public class JobProvider { result -> handler.accept(result.result), errorHandler, () -> null); } - public void updateCalendar(String calendarId, Set jobIdsToAdd, Set jobIdsToRemove, MlMetadata mlMetadata, + public void updateCalendar(String calendarId, Set jobIdsToAdd, Set jobIdsToRemove, Consumer handler, Consumer errorHandler) { ActionListener getCalendarListener = ActionListener.wrap( diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java index 11f714ae449..120f04e95e7 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java @@ -244,7 +244,7 @@ public class JobProviderIT extends MlSingleNodeTestCase { throws Exception { CountDownLatch latch = new CountDownLatch(1); AtomicReference exceptionHolder = new AtomicReference<>(); - jobProvider.updateCalendar(calendarId, idsToAdd, idsToRemove, mlMetadata, + jobProvider.updateCalendar(calendarId, idsToAdd, idsToRemove, r -> latch.countDown(), e -> { exceptionHolder.set(e); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml index 9b3572739cd..1406e04c8da 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml @@ -86,23 +86,6 @@ xpack.ml.get_calendars: calendar_id: "dogs_of_the_year" - - do: - xpack.ml.put_calendar: - calendar_id: "new_cal_with_unknown_job_group" - body: > - { - "job_ids": ["cal-job", "unknown-job-group"] - } - - - do: - xpack.ml.get_calendars: - calendar_id: "new_cal_with_unknown_job_group" - - match: { count: 1 } - - match: - calendars.0: - calendar_id: "new_cal_with_unknown_job_group" - job_ids: ["cal-job", "unknown-job-group"] - --- "Test get calendar given missing": - do: @@ -714,3 +697,106 @@ - match: { calendar_id: "expression" } - length: { job_ids: 1 } - match: { job_ids.0: "bar-a" } + +--- +"Test calendar actions with new job group": + - do: + xpack.ml.put_job: + job_id: calendar-job + body: > + { + "analysis_config" : { + "detectors" :[{"function":"metric","field_name":"responsetime","by_field_name":"airline"}] + }, + "data_description" : { + } + } + + - do: + xpack.ml.put_calendar: + calendar_id: "cal_with_new_job_group" + body: > + { + "job_ids": ["calendar-job", "new-job-group"] + } + + - do: + xpack.ml.get_calendars: + calendar_id: "cal_with_new_job_group" + - match: { count: 1 } + - match: + calendars.0: + calendar_id: "cal_with_new_job_group" + job_ids: ["calendar-job", "new-job-group"] + + - do: + xpack.ml.post_calendar_events: + calendar_id: "cal_with_new_job_group" + body: > + { + "events" : [{ "description": "beach", "start_time": "2018-05-01T00:00:00Z", "end_time": "2018-05-06T00:00:00Z" }] + } + + - do: + xpack.ml.get_calendar_events: + calendar_id: cal_with_new_job_group + - length: { events: 1 } + - match: { events.0.description: beach } + + - do: + xpack.ml.delete_calendar: + calendar_id: "cal_with_new_job_group" + + - do: + xpack.ml.put_calendar: + calendar_id: "started_empty_calendar" + + - do: + xpack.ml.put_calendar_job: + calendar_id: "started_empty_calendar" + job_id: "new-group" + - match: { calendar_id: "started_empty_calendar" } + - length: { job_ids: 1 } + + - do: + xpack.ml.get_calendars: + calendar_id: "started_empty_calendar" + - match: { count: 1 } + - match: + calendars.0: + calendar_id: "started_empty_calendar" + job_ids: ["new-group"] + + - do: + xpack.ml.post_calendar_events: + calendar_id: "started_empty_calendar" + body: > + { + "events" : [{ "description": "beach", "start_time": "2018-05-01T00:00:00Z", "end_time": "2018-05-06T00:00:00Z" }] + } + + - do: + xpack.ml.get_calendar_events: + calendar_id: "started_empty_calendar" + - length: { events: 1 } + - match: { events.0.description: beach } + - set: { events.0.event_id: beach_event_id } + + - do: + xpack.ml.delete_calendar_event: + calendar_id: "started_empty_calendar" + event_id: $beach_event_id + + - do: + xpack.ml.get_calendar_events: + calendar_id: "started_empty_calendar" + - length: { events: 0 } + + - do: + xpack.ml.delete_calendar: + calendar_id: "started_empty_calendar" + + - do: + catch: missing + xpack.ml.get_calendars: + calendar_id: "started_empty_calendar"