From 2d01c3884b2c4900f44c0f07fd65c188ef802247 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Mon, 3 Apr 2017 11:04:34 +0100 Subject: [PATCH] [ML] Don't get stats for jobs that are being deleted (elastic/x-pack-elasticsearch#899) If jobs are being deleted then the operations required to get stats could fail with unexpected exceptions. When stats for multiple jobs were being requested, this would previously cause the whole operation to fail. This commit changes the stats endpoint to ignore jobs that are being deleted. Fixes elastic/prelert-legacy#837 Original commit: elastic/x-pack-elasticsearch@6ac141a9877a72be22529594add779412443d676 --- .../elasticsearch/xpack/ml/MlMetadata.java | 5 ++ .../xpack/ml/action/GetJobsStatsAction.java | 16 +++++-- .../ml/action/GetJobsStatsActionTests.java | 46 +++++++++++++------ 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/MlMetadata.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/MlMetadata.java index f13c59cf889..436b0e9f9bc 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/MlMetadata.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/MlMetadata.java @@ -73,6 +73,11 @@ public class MlMetadata implements MetaData.Custom { return jobs; } + public boolean isJobDeleted(String jobId) { + Job job = jobs.get(jobId); + return job == null || job.isDeleted(); + } + public SortedMap getDatafeeds() { return datafeeds; } diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/action/GetJobsStatsAction.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/action/GetJobsStatsAction.java index fb860cf96f9..69d8f853841 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/action/GetJobsStatsAction.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/action/GetJobsStatsAction.java @@ -379,7 +379,8 @@ public class GetJobsStatsAction extends Action finalListener = listener; - listener = ActionListener.wrap(response -> gatherStatsForClosedJobs(request, response, finalListener), listener::onFailure); + listener = ActionListener.wrap(response -> gatherStatsForClosedJobs(mlMetadata, + request, response, finalListener), listener::onFailure); super.doExecute(task, request, listener); } @@ -428,8 +429,10 @@ public class GetJobsStatsAction extends Action listener) { - List jobIds = determineJobIdsWithoutLiveStats(request.expandedJobsIds, response.jobsStats.results()); + void gatherStatsForClosedJobs(MlMetadata mlMetadata, Request request, Response response, + ActionListener listener) { + List jobIds = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, + request.expandedJobsIds, response.jobsStats.results()); if (jobIds.isEmpty()) { listener.onResponse(response); return; @@ -477,9 +480,12 @@ public class GetJobsStatsAction extends Action determineJobIdsWithoutLiveStats(List requestedJobIds, List stats) { + static List determineNonDeletedJobIdsWithoutLiveStats(MlMetadata mlMetadata, + List requestedJobIds, + List stats) { Set excludeJobIds = stats.stream().map(Response.JobStats::getJobId).collect(Collectors.toSet()); - return requestedJobIds.stream().filter(jobId -> !excludeJobIds.contains(jobId)).collect(Collectors.toList()); + return requestedJobIds.stream().filter(jobId -> !excludeJobIds.contains(jobId) && + !mlMetadata.isJobDeleted(jobId)).collect(Collectors.toList()); } } } diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsStatsActionTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsStatsActionTests.java index 796b72fff66..6000d5845c2 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsStatsActionTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetJobsStatsActionTests.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.ml.action; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ml.MlMetadata; import org.elasticsearch.xpack.ml.job.config.JobState; import org.elasticsearch.xpack.ml.job.process.autodetect.state.DataCounts; @@ -16,27 +17,37 @@ import java.util.Collections; import java.util.List; import java.util.Optional; -import static org.elasticsearch.xpack.ml.action.GetJobsStatsAction.TransportAction.determineJobIdsWithoutLiveStats; +import static org.elasticsearch.xpack.ml.action.GetJobsStatsAction.TransportAction.determineNonDeletedJobIdsWithoutLiveStats; + +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class GetJobsStatsActionTests extends ESTestCase { public void testDetermineJobIds() { - List result = determineJobIdsWithoutLiveStats(Collections.singletonList("id1"), Collections.emptyList()); + + MlMetadata mlMetadata = mock(MlMetadata.class); + when(mlMetadata.isJobDeleted(eq("id4"))).thenReturn(true); + + List result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, + Collections.singletonList("id1"), Collections.emptyList()); assertEquals(1, result.size()); assertEquals("id1", result.get(0)); - result = determineJobIdsWithoutLiveStats(Collections.singletonList("id1"), Collections.singletonList( - new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.CLOSED, null, null, null))); + result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, + Collections.singletonList("id1"), Collections.singletonList( + new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.OPENED, null, null, null))); assertEquals(0, result.size()); - result = determineJobIdsWithoutLiveStats( + result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, Arrays.asList("id1", "id2", "id3"), Collections.emptyList()); assertEquals(3, result.size()); assertEquals("id1", result.get(0)); assertEquals("id2", result.get(1)); assertEquals("id3", result.get(2)); - result = determineJobIdsWithoutLiveStats( + result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, Arrays.asList("id1", "id2", "id3"), Collections.singletonList(new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.CLOSED, null, null, null)) @@ -45,19 +56,28 @@ public class GetJobsStatsActionTests extends ESTestCase { assertEquals("id2", result.get(0)); assertEquals("id3", result.get(1)); - result = determineJobIdsWithoutLiveStats(Arrays.asList("id1", "id2", "id3"), Arrays.asList( - new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.CLOSED, null, null, null), - new GetJobsStatsAction.Response.JobStats("id3", new DataCounts("id3"), null, JobState.CLOSED, null, null, null) + result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, + Arrays.asList("id1", "id2", "id3"), Arrays.asList( + new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.OPENED, null, null, null), + new GetJobsStatsAction.Response.JobStats("id3", new DataCounts("id3"), null, JobState.OPENED, null, null, null) )); assertEquals(1, result.size()); assertEquals("id2", result.get(0)); - result = determineJobIdsWithoutLiveStats(Arrays.asList("id1", "id2", "id3"), + result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, Arrays.asList("id1", "id2", "id3"), Arrays.asList( - new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.CLOSED, null, null, null), - new GetJobsStatsAction.Response.JobStats("id2", new DataCounts("id2"), null, JobState.CLOSED, null, null, null), - new GetJobsStatsAction.Response.JobStats("id3", new DataCounts("id3"), null, JobState.CLOSED, null, null, null))); + new GetJobsStatsAction.Response.JobStats("id1", new DataCounts("id1"), null, JobState.OPENED, null, null, null), + new GetJobsStatsAction.Response.JobStats("id2", new DataCounts("id2"), null, JobState.OPENED, null, null, null), + new GetJobsStatsAction.Response.JobStats("id3", new DataCounts("id3"), null, JobState.OPENED, null, null, null))); assertEquals(0, result.size()); + + // No jobs running, but job 4 is being deleted + result = determineNonDeletedJobIdsWithoutLiveStats(mlMetadata, + Arrays.asList("id1", "id2", "id3", "id4"), Collections.emptyList()); + assertEquals(3, result.size()); + assertEquals("id1", result.get(0)); + assertEquals("id2", result.get(1)); + assertEquals("id3", result.get(2)); } public void testDurationToTimeValue() {