From 96927cc1b6450da22ce9eeb83842f7066a02c49c Mon Sep 17 00:00:00 2001
From: David Roberts <dave.roberts@elastic.co>
Date: Tue, 20 Jun 2017 13:06:28 +0100
Subject: [PATCH] [ML] Handle failures in the idiomatic way
 (elastic/x-pack-elasticsearch#1785)

This commit changes a couple of places where our ExceptionsHelper
class was throwing exceptions to instead return the exceptions.
Then they can be passed to onFailure() methods or thrown depending
on what's appropriate for the caller.  This is the standard Elastic
way of handling failures.

Original commit: elastic/x-pack-elasticsearch@fce07eb075a03fa8642fcb3221c6b493a13029be
---
 .../xpack/ml/job/JobManager.java              |  1 +
 .../xpack/ml/utils/ExceptionsHelper.java      |  4 ++--
 .../xpack/ml/job/JobManagerTests.java         | 21 +++++++++----------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java
index a419b9eeef2..4c6ab0e9ca8 100644
--- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java
+++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java
@@ -167,6 +167,7 @@ public class JobManager extends AbstractComponent {
         MlMetadata currentMlMetadata = state.metaData().custom(MlMetadata.TYPE);
         if (currentMlMetadata != null && currentMlMetadata.getJobs().containsKey(job.getId())) {
             actionListener.onFailure(ExceptionsHelper.jobAlreadyExists(job.getId()));
+            return;
         }
 
         ActionListener<Boolean> putJobListener = new ActionListener<Boolean>() {
diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/utils/ExceptionsHelper.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/utils/ExceptionsHelper.java
index ead25a48455..b1d0655ceae 100644
--- a/plugin/src/main/java/org/elasticsearch/xpack/ml/utils/ExceptionsHelper.java
+++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/utils/ExceptionsHelper.java
@@ -23,11 +23,11 @@ public class ExceptionsHelper {
     }
 
     public static ResourceAlreadyExistsException jobAlreadyExists(String jobId) {
-        throw new ResourceAlreadyExistsException(Messages.getMessage(Messages.JOB_CONFIG_ID_ALREADY_TAKEN, jobId));
+        return new ResourceAlreadyExistsException(Messages.getMessage(Messages.JOB_CONFIG_ID_ALREADY_TAKEN, jobId));
     }
 
     public static ResourceNotFoundException missingDatafeedException(String datafeedId) {
-        throw new ResourceNotFoundException(Messages.getMessage(Messages.DATAFEED_NOT_FOUND, datafeedId));
+        return new ResourceNotFoundException(Messages.getMessage(Messages.DATAFEED_NOT_FOUND, datafeedId));
     }
 
     public static ElasticsearchException serverError(String msg) {
diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java
index 5606ad8abcc..7723d1b30d7 100644
--- a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java
+++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java
@@ -167,18 +167,17 @@ public class JobManagerTests extends ESTestCase {
         ClusterState clusterState = ClusterState.builder(new ClusterName("name"))
                 .metaData(MetaData.builder().putCustom(MlMetadata.TYPE, mlMetadata.build())).build();
 
-        expectThrows(ResourceAlreadyExistsException.class, () ->
-                jobManager.putJob(putJobRequest, clusterState, new ActionListener<PutJobAction.Response>() {
-                        @Override
-                        public void onResponse(PutJobAction.Response response) {
+        jobManager.putJob(putJobRequest, clusterState, new ActionListener<PutJobAction.Response>() {
+            @Override
+            public void onResponse(PutJobAction.Response response) {
+                fail("should have got an error");
+            }
 
-                        }
-
-                        @Override
-                        public void onFailure(Exception e) {
-                            fail(e.toString());
-                        }
-                    }));
+            @Override
+            public void onFailure(Exception e) {
+                assertTrue(e instanceof ResourceAlreadyExistsException);
+            }
+        });
     }
 
     private Job.Builder createJob() {