Merge pull request #366 from metamx/fix-terminate

Indexing service: fix the edge case where autoscaling tries to terminate node without an assigned IP

indexing-service/src/main/java/io/druid/indexing/overlord/scaling/AutoScalingStrategy.java

@@ -30,6 +30,8 @@ public interface AutoScalingStrategy
 
   public AutoScalingData terminate(List<String> ips);
 
+  public AutoScalingData terminateWithIds(List<String> ids);
+
   /**
    * Provides a lookup of ip addresses to node ids
    * @param ips - nodes IPs

indexing-service/src/main/java/io/druid/indexing/overlord/scaling/EC2AutoScalingStrategy.java

@@ -155,32 +155,15 @@ public class EC2AutoScalingStrategy implements AutoScalingStrategy
     }
 
     try {
-      log.info("Terminating instance[%s]", instances);
-      amazonEC2Client.terminateInstances(
-          new TerminateInstancesRequest(
-              Lists.transform(
-                  instances,
-                  new Function<Instance, String>()
-                  {
-                    @Override
-                    public String apply(Instance input)
-                    {
-                      return input.getInstanceId();
-                    }
-                  }
-              )
-          )
-      );
-
-      return new AutoScalingData(
+      return terminateWithIds(
           Lists.transform(
-              ips,
-              new Function<String, String>()
+              instances,
+              new Function<Instance, String>()
               {
                 @Override
-                public String apply(@Nullable String input)
+                public String apply(Instance input)
                 {
-                  return String.format("%s:%s", input, config.getWorkerPort());
+                  return input.getInstanceId();
                 }
               }
           )
@@ -193,6 +176,28 @@ public class EC2AutoScalingStrategy implements AutoScalingStrategy
     return null;
   }
 
+  @Override
+  public AutoScalingData terminateWithIds(List<String> ids)
+  {
+    if (ids.isEmpty()) {
+      return new AutoScalingData(Lists.<String>newArrayList());
+    }
+
+    try {
+      log.info("Terminating instances[%s]", ids);
+      amazonEC2Client.terminateInstances(
+          new TerminateInstancesRequest(ids)
+      );
+
+      return new AutoScalingData(ids);
+    }
+    catch (Exception e) {
+      log.error(e, "Unable to terminate any instances.");
+    }
+
+    return null;
+  }
+
   @Override
   public List<String> ipToIdLookup(List<String> ips)
   {

indexing-service/src/main/java/io/druid/indexing/overlord/scaling/NoopAutoScalingStrategy.java

@@ -44,6 +44,13 @@ public class NoopAutoScalingStrategy implements AutoScalingStrategy
     return null;
   }
 
+  @Override
+  public AutoScalingData terminateWithIds(List<String> ids)
+  {
+    log.info("If I were a real strategy I'd terminate %s now", ids);
+    return null;
+  }
+
   @Override
   public List<String> ipToIdLookup(List<String> ips)
   {

indexing-service/src/main/java/io/druid/indexing/overlord/scaling/SimpleResourceManagementStrategy.java

@@ -132,8 +132,7 @@ public class SimpleResourceManagementStrategy implements ResourceManagementStrat
              .addData("provisioningCount", currentlyProvisioning.size())
              .emit();
 
-          List<String> nodeIps = autoScalingStrategy.idToIpLookup(Lists.newArrayList(currentlyProvisioning));
-          autoScalingStrategy.terminate(nodeIps);
+          autoScalingStrategy.terminateWithIds(Lists.newArrayList(currentlyProvisioning));
           currentlyProvisioning.clear();
         }
       }

indexing-service/src/test/java/io/druid/indexing/overlord/scaling/SimpleResourceManagementStrategyTest.java

@@ -187,9 +187,7 @@ public class SimpleResourceManagementStrategyTest
 
     EasyMock.expect(autoScalingStrategy.ipToIdLookup(EasyMock.<List<String>>anyObject()))
             .andReturn(Lists.<String>newArrayList()).times(2);
-    EasyMock.expect(autoScalingStrategy.idToIpLookup(EasyMock.<List<String>>anyObject()))
-                .andReturn(Lists.<String>newArrayList());
-    EasyMock.expect(autoScalingStrategy.terminate(EasyMock.<List<String>>anyObject()))
+    EasyMock.expect(autoScalingStrategy.terminateWithIds(EasyMock.<List<String>>anyObject()))
                     .andReturn(null);
     EasyMock.expect(autoScalingStrategy.provision()).andReturn(
         new AutoScalingData(Lists.<String>newArrayList("fake"))

indexing-service/src/test/java/io/druid/indexing/overlord/scaling/TestAutoScalingStrategy.java

@@ -1,51 +0,0 @@
-/*
- * Druid - a distributed column store.
- * Copyright (C) 2012, 2013  Metamarkets Group Inc.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- */
-
-package io.druid.indexing.overlord.scaling;
-
-import java.util.List;
-
-/**
- */
-public class TestAutoScalingStrategy<T> implements AutoScalingStrategy
-{
-  @Override
-  public AutoScalingData provision()
-  {
-    return null;
-  }
-
-  @Override
-  public AutoScalingData terminate(List<String> ips)
-  {
-    return null;
-  }
-
-  @Override
-  public List<String> ipToIdLookup(List<String> ips)
-  {
-    return null;
-  }
-
-  @Override
-  public List<String> idToIpLookup(List<String> nodeIds)
-  {
-    return null;
-  }
-}