[ML] Updating detectors should not affect existing job (#32040)

This is a light backport of #31957 for 6.3.
elastic · Jul 13, 2018 · eae6fed · eae6fed
1 parent bda7033
commit eae6fed
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 11 deletions.
diff --git a/.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfig.java b/.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfig.java
@@ -487,7 +487,7 @@ public Builder(List<Detector> detectors) {
         }
 
         public Builder(AnalysisConfig analysisConfig) {
-            this.detectors = analysisConfig.detectors;
+            this.detectors = new ArrayList<>(analysisConfig.detectors);
             this.bucketSpan = analysisConfig.bucketSpan;
             this.latency = analysisConfig.latency;
             this.categorizationFieldName = analysisConfig.categorizationFieldName;
@@ -518,6 +518,10 @@ public void setDetectors(List<Detector> detectors) {
             this.detectors = sequentialIndexDetectors;
         }
 
+        public void setDetector(int detectorIndex, Detector detector) {
+            detectors.set(detectorIndex, detector);
+        }
+
         public void setBucketSpan(TimeValue bucketSpan) {
             this.bucketSpan = bucketSpan;
         }

diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java
@@ -346,33 +346,31 @@ public Set<String> getUpdateFields() {
      */
     public Job mergeWithJob(Job source, ByteSizeValue maxModelMemoryLimit) {
         Job.Builder builder = new Job.Builder(source);
+        AnalysisConfig currentAnalysisConfig = source.getAnalysisConfig();
+        AnalysisConfig.Builder newAnalysisConfig = new AnalysisConfig.Builder(currentAnalysisConfig);
         if (groups != null) {
             builder.setGroups(groups);
         }
         if (description != null) {
             builder.setDescription(description);
         }
         if (detectorUpdates != null && detectorUpdates.isEmpty() == false) {
-            AnalysisConfig ac = source.getAnalysisConfig();
-            int numDetectors = ac.getDetectors().size();
+            int numDetectors = currentAnalysisConfig.getDetectors().size();
             for (DetectorUpdate dd : detectorUpdates) {
                 if (dd.getDetectorIndex() >= numDetectors) {
                     throw ExceptionsHelper.badRequestException("Supplied detector_index [{}] is >= the number of detectors [{}]",
                             dd.getDetectorIndex(), numDetectors);
                 }
 
-                Detector.Builder detectorbuilder = new Detector.Builder(ac.getDetectors().get(dd.getDetectorIndex()));
+                Detector.Builder detectorbuilder = new Detector.Builder(currentAnalysisConfig.getDetectors().get(dd.getDetectorIndex()));
                 if (dd.getDescription() != null) {
                     detectorbuilder.setDetectorDescription(dd.getDescription());
                 }
                 if (dd.getRules() != null) {
                     detectorbuilder.setRules(dd.getRules());
                 }
-                ac.getDetectors().set(dd.getDetectorIndex(), detectorbuilder.build());
+                newAnalysisConfig.setDetector(dd.getDetectorIndex(), detectorbuilder.build());
             }
-
-            AnalysisConfig.Builder acBuilder = new AnalysisConfig.Builder(ac);
-            builder.setAnalysisConfig(acBuilder);
         }
         if (modelPlotConfig != null) {
             builder.setModelPlotConfig(modelPlotConfig);
@@ -395,9 +393,7 @@ public Job mergeWithJob(Job source, ByteSizeValue maxModelMemoryLimit) {
             builder.setResultsRetentionDays(resultsRetentionDays);
         }
         if (categorizationFilters != null) {
-            AnalysisConfig.Builder analysisConfigBuilder = new AnalysisConfig.Builder(source.getAnalysisConfig());
-            analysisConfigBuilder.setCategorizationFilters(categorizationFilters);
-            builder.setAnalysisConfig(analysisConfigBuilder);
+            newAnalysisConfig.setCategorizationFilters(categorizationFilters);
         }
         if (customSettings != null) {
             builder.setCustomSettings(customSettings);
@@ -416,6 +412,7 @@ public Job mergeWithJob(Job source, ByteSizeValue maxModelMemoryLimit) {
         if (jobVersion != null) {
             builder.setJobVersion(jobVersion);
         }
+        builder.setAnalysisConfig(newAnalysisConfig);
         return builder.build();
     }
 

diff --git a/.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java b/.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java
@@ -22,6 +22,7 @@
 import java.util.Map;
 
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 import static org.mockito.Mockito.mock;
 
 public class JobUpdateTests extends AbstractSerializingTestCase<JobUpdate> {
@@ -182,6 +183,28 @@ public void testMergeWithJob() {
         }
     }
 
+    public void testDetectorUpdate_DoesNotAffectOriginalJob() {
+        Job.Builder jobBuilder = new Job.Builder("foo");
+        Detector.Builder detector = new Detector.Builder("count", null);
+        AnalysisConfig.Builder ac = new AnalysisConfig.Builder(Collections.singletonList(detector.build()));
+        jobBuilder.setAnalysisConfig(ac);
+        jobBuilder.setDataDescription(new DataDescription.Builder());
+        jobBuilder.setCreateTime(new Date());
+        Job job = jobBuilder.build();
+
+        List<DetectionRule> rules = Collections.singletonList(new DetectionRule.Builder(Collections.singletonList(
+                new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, new Condition(Operator.GT, "5"), null))).build());
+        JobUpdate.DetectorUpdate detectorUpdate = new JobUpdate.DetectorUpdate(0, null, rules);
+        JobUpdate.Builder updateBuilder = new JobUpdate.Builder("foo");
+        updateBuilder.setDetectorUpdates(Collections.singletonList(detectorUpdate));
+        JobUpdate update = updateBuilder.build();
+
+        Job updatedJob = update.mergeWithJob(job, new ByteSizeValue(0L));
+
+        assertThat(updatedJob.getAnalysisConfig().getDetectors().get(0).getRules(), equalTo(rules));
+        assertThat(job.getAnalysisConfig().getDetectors().get(0).getRules().isEmpty(), is(true));
+    }
+
     public void testIsAutodetectProcessUpdate() {
         JobUpdate update = new JobUpdate.Builder("foo").build();
         assertFalse(update.isAutodetectProcessUpdate());