HADOOP-8193. Refactor FailoverController/HAAdmin code to add an abstract class for "target" services. Contributed by Todd Lipcon.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.23@1305196 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -109,6 +109,9 @@ Release 0.23.3 - UNRELEASED
     HADOOP-8163. Improve ActiveStandbyElector to provide hooks for
     fencing old active. (todd)
 
+    HADOOP-8193. Refactor FailoverController/HAAdmin code to add an abstract
+    class for "target" services. (todd)
+
   OPTIMIZATIONS
 
   BUG FIXES

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/BadFencingConfigurationException.java

@@ -19,11 +19,16 @@ package org.apache.hadoop.ha;
 
 import java.io.IOException;
 
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+
 /**
  * Indicates that the operator has specified an invalid configuration
  * for fencing methods.
  */
-class BadFencingConfigurationException extends IOException {
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public class BadFencingConfigurationException extends IOException {
   private static final long serialVersionUID = 1L;
 
   public BadFencingConfigurationException(String msg) {

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/FailoverController.java

@@ -18,7 +18,6 @@
 package org.apache.hadoop.ha;
 
 import java.io.IOException;
-import java.net.InetSocketAddress;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -51,21 +50,21 @@ public class FailoverController {
    * allow it to become active, eg because it triggers a log roll
    * so the standby can learn about new blocks and leave safemode.
    *
-   * @param toSvc service to make active
+   * @param target service to make active
-   * @param toSvcName name of service to make active
    * @param forceActive ignore toSvc if it reports that it is not ready
    * @throws FailoverFailedException if we should avoid failover
    */
-  private static void preFailoverChecks(HAServiceProtocol toSvc,
+  private static void preFailoverChecks(HAServiceTarget target,
-                                        InetSocketAddress toSvcAddr,
                                         boolean forceActive)
       throws FailoverFailedException {
     HAServiceStatus toSvcStatus;
+    HAServiceProtocol toSvc;
 
     try {
+      toSvc = target.getProxy();
       toSvcStatus = toSvc.getServiceStatus();
     } catch (IOException e) {
-      String msg = "Unable to get service state for " + toSvcAddr;
+      String msg = "Unable to get service state for " + target;
       LOG.error(msg, e);
       throw new FailoverFailedException(msg, e);
     }
@@ -79,7 +78,7 @@ public class FailoverController {
       String notReadyReason = toSvcStatus.getNotReadyReason();
       if (!forceActive) {
         throw new FailoverFailedException(
-            toSvcAddr + " is not ready to become active: " +
+            target + " is not ready to become active: " +
             notReadyReason);
       } else {
         LOG.warn("Service is not ready to become active, but forcing: " +
@@ -103,44 +102,39 @@ public class FailoverController {
    * then try to failback.
    *
    * @param fromSvc currently active service
-   * @param fromSvcAddr addr of the currently active service
    * @param toSvc service to make active
-   * @param toSvcAddr addr of the service to make active
-   * @param fencer for fencing fromSvc
    * @param forceFence to fence fromSvc even if not strictly necessary
    * @param forceActive try to make toSvc active even if it is not ready
    * @throws FailoverFailedException if the failover fails
    */
-  public static void failover(HAServiceProtocol fromSvc,
+  public static void failover(HAServiceTarget fromSvc,
-                              InetSocketAddress fromSvcAddr,
+                              HAServiceTarget toSvc,
-                              HAServiceProtocol toSvc,
-                              InetSocketAddress toSvcAddr,
-                              NodeFencer fencer,
                               boolean forceFence,
                               boolean forceActive)
       throws FailoverFailedException {
-    Preconditions.checkArgument(fencer != null, "failover requires a fencer");
+    Preconditions.checkArgument(fromSvc.getFencer() != null,
-    preFailoverChecks(toSvc, toSvcAddr, forceActive);
+        "failover requires a fencer");
+    preFailoverChecks(toSvc, forceActive);
 
     // Try to make fromSvc standby
     boolean tryFence = true;
     try {
-      HAServiceProtocolHelper.transitionToStandby(fromSvc);
+      HAServiceProtocolHelper.transitionToStandby(fromSvc.getProxy());
       // We should try to fence if we failed or it was forced
       tryFence = forceFence ? true : false;
     } catch (ServiceFailedException sfe) {
-      LOG.warn("Unable to make " + fromSvcAddr + " standby (" +
+      LOG.warn("Unable to make " + fromSvc + " standby (" +
           sfe.getMessage() + ")");
     } catch (IOException ioe) {
-      LOG.warn("Unable to make " + fromSvcAddr +
+      LOG.warn("Unable to make " + fromSvc +
           " standby (unable to connect)", ioe);
     }
 
     // Fence fromSvc if it's required or forced by the user
     if (tryFence) {
-      if (!fencer.fence(fromSvcAddr)) {
+      if (!fromSvc.getFencer().fence(fromSvc)) {
         throw new FailoverFailedException("Unable to fence " +
-            fromSvcAddr + ". Fencing failed.");
+            fromSvc + ". Fencing failed.");
       }
     }
 
@@ -148,14 +142,14 @@ public class FailoverController {
     boolean failed = false;
     Throwable cause = null;
     try {
-      HAServiceProtocolHelper.transitionToActive(toSvc);
+      HAServiceProtocolHelper.transitionToActive(toSvc.getProxy());
     } catch (ServiceFailedException sfe) {
-      LOG.error("Unable to make " + toSvcAddr + " active (" +
+      LOG.error("Unable to make " + toSvc + " active (" +
           sfe.getMessage() + "). Failing back.");
       failed = true;
       cause = sfe;
     } catch (IOException ioe) {
-      LOG.error("Unable to make " + toSvcAddr +
+      LOG.error("Unable to make " + toSvc +
           " active (unable to connect). Failing back.", ioe);
       failed = true;
       cause = ioe;
@@ -163,7 +157,7 @@ public class FailoverController {
 
     // We failed to make toSvc active
     if (failed) {
-      String msg = "Unable to failover to " + toSvcAddr;
+      String msg = "Unable to failover to " + toSvc;
       // Only try to failback if we didn't fence fromSvc
       if (!tryFence) {
         try {
@@ -171,9 +165,9 @@ public class FailoverController {
           // become active, eg we timed out waiting for its response.
           // Unconditionally force fromSvc to become active since it
           // was previously active when we initiated failover.
-          failover(toSvc, toSvcAddr, fromSvc, fromSvcAddr, fencer, true, true);
+          failover(toSvc, fromSvc, true, true);
         } catch (FailoverFailedException ffe) {
-          msg += ". Failback to " + fromSvcAddr +
+          msg += ". Failback to " + fromSvc +
             " failed (" + ffe.getMessage() + ")";
           LOG.fatal(msg);
         }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/FenceMethod.java

@@ -17,8 +17,6 @@
  */
 package org.apache.hadoop.ha;
 
-import java.net.InetSocketAddress;
-
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.conf.Configurable;
@@ -62,6 +60,6 @@ public interface FenceMethod {
    * @throws BadFencingConfigurationException if the configuration was
    *         determined to be invalid only at runtime
    */
-  public boolean tryFence(InetSocketAddress serviceAddr, String args)
+  public boolean tryFence(HAServiceTarget target, String args)
     throws BadFencingConfigurationException;
 }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/HAAdmin.java

@@ -19,7 +19,6 @@ package org.apache.hadoop.ha;
 
 import java.io.IOException;
 import java.io.PrintStream;
-import java.net.InetSocketAddress;
 import java.util.Map;
 
 import org.apache.commons.cli.Options;
@@ -27,12 +26,11 @@ import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.CommandLineParser;
 import org.apache.commons.cli.GnuParser;
 import org.apache.commons.cli.ParseException;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
-import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.conf.Configured;
-import org.apache.hadoop.ha.protocolPB.HAServiceProtocolClientSideTranslatorPB;
-import org.apache.hadoop.net.NetUtils;
 import org.apache.hadoop.util.Tool;
 import org.apache.hadoop.util.ToolRunner;
 
@@ -49,6 +47,7 @@ public abstract class HAAdmin extends Configured implements Tool {
   
   private static final String FORCEFENCE  = "forcefence";
   private static final String FORCEACTIVE = "forceactive";
+  private static final Log LOG = LogFactory.getLog(HAAdmin.class);
 
   private static Map<String, UsageInfo> USAGE =
     ImmutableMap.<String, UsageInfo>builder()
@@ -77,6 +76,8 @@ public abstract class HAAdmin extends Configured implements Tool {
   protected PrintStream errOut = System.err;
   PrintStream out = System.out;
 
+  protected abstract HAServiceTarget resolveTarget(String string);
+
   protected String getUsageString() {
     return "Usage: HAAdmin";
   }
@@ -109,7 +110,7 @@ public abstract class HAAdmin extends Configured implements Tool {
       return -1;
     }
     
-    HAServiceProtocol proto = getProtocol(argv[1]);
+    HAServiceProtocol proto = resolveTarget(argv[1]).getProxy();
     HAServiceProtocolHelper.transitionToActive(proto);
     return 0;
   }
@@ -122,14 +123,13 @@ public abstract class HAAdmin extends Configured implements Tool {
       return -1;
     }
     
-    HAServiceProtocol proto = getProtocol(argv[1]);
+    HAServiceProtocol proto = resolveTarget(argv[1]).getProxy();
     HAServiceProtocolHelper.transitionToStandby(proto);
     return 0;
   }
 
   private int failover(final String[] argv)
       throws IOException, ServiceFailedException {
-    Configuration conf = getConf();
     boolean forceFence = false;
     boolean forceActive = false;
 
@@ -162,29 +162,12 @@ public abstract class HAAdmin extends Configured implements Tool {
       return -1;
     }
 
-    NodeFencer fencer;
+    HAServiceTarget fromNode = resolveTarget(args[0]);
+    HAServiceTarget toNode = resolveTarget(args[1]);
+    
     try {
-      fencer = NodeFencer.create(conf);
+      FailoverController.failover(fromNode, toNode,
-    } catch (BadFencingConfigurationException bfce) {
+          forceFence, forceActive); 
-      errOut.println("failover: incorrect fencing configuration: " + 
-          bfce.getLocalizedMessage());
-      return -1;
-    }
-    if (fencer == null) {
-      errOut.println("failover: no fencer configured");
-      return -1;
-    }
-
-    InetSocketAddress addr1 = 
-      NetUtils.createSocketAddr(getServiceAddr(args[0]));
-    InetSocketAddress addr2 = 
-      NetUtils.createSocketAddr(getServiceAddr(args[1]));
-    HAServiceProtocol proto1 = getProtocol(args[0]);
-    HAServiceProtocol proto2 = getProtocol(args[1]);
-
-    try {
-      FailoverController.failover(proto1, addr1, proto2, addr2,
-          fencer, forceFence, forceActive); 
       out.println("Failover from "+args[0]+" to "+args[1]+" successful");
     } catch (FailoverFailedException ffe) {
       errOut.println("Failover failed: " + ffe.getLocalizedMessage());
@@ -201,7 +184,7 @@ public abstract class HAAdmin extends Configured implements Tool {
       return -1;
     }
     
-    HAServiceProtocol proto = getProtocol(argv[1]);
+    HAServiceProtocol proto = resolveTarget(argv[1]).getProxy();
     try {
       HAServiceProtocolHelper.monitorHealth(proto);
     } catch (HealthCheckFailedException e) {
@@ -219,7 +202,7 @@ public abstract class HAAdmin extends Configured implements Tool {
       return -1;
     }
 
-    HAServiceProtocol proto = getProtocol(argv[1]);
+    HAServiceProtocol proto = resolveTarget(argv[1]).getProxy();
     out.println(proto.getServiceStatus().getState());
     return 0;
   }
@@ -232,16 +215,6 @@ public abstract class HAAdmin extends Configured implements Tool {
     return serviceId;
   }
 
-  /**
-   * Return a proxy to the specified target service.
-   */
-  protected HAServiceProtocol getProtocol(String serviceId)
-      throws IOException {
-    String serviceAddr = getServiceAddr(serviceId);
-    InetSocketAddress addr = NetUtils.createSocketAddr(serviceAddr);
-    return new HAServiceProtocolClientSideTranslatorPB(addr, getConf());
-  }
-
   @Override
   public int run(String[] argv) throws Exception {
     try {
@@ -251,6 +224,9 @@ public abstract class HAAdmin extends Configured implements Tool {
       return -1;
     } catch (IOException ioe) {
       errOut.println("Operation failed: " + ioe.getLocalizedMessage());
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Operation failed", ioe);
+      }
       return -1;
     }
   }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/HAServiceTarget.java

@@ -0,0 +1,78 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ha;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.ha.protocolPB.HAServiceProtocolClientSideTranslatorPB;
+import org.apache.hadoop.net.NetUtils;
+
+/**
+ * Represents a target of the client side HA administration commands.
+ */
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public abstract class HAServiceTarget {
+
+  /**
+   * @return the IPC address of the target node.
+   */
+  public abstract InetSocketAddress getAddress();
+
+  /**
+   * @return a Fencer implementation configured for this target node
+   */
+  public abstract NodeFencer getFencer();
+  
+  /**
+   * @throws BadFencingConfigurationException if the fencing configuration
+   * appears to be invalid. This is divorced from the above
+   * {@link #getFencer()} method so that the configuration can be checked
+   * during the pre-flight phase of failover.
+   */
+  public abstract void checkFencingConfigured()
+      throws BadFencingConfigurationException;
+  
+  /**
+   * @return a proxy to connect to the target HA Service.
+   */
+  public HAServiceProtocol getProxy(Configuration conf, int timeoutMs)
+      throws IOException {
+    Configuration confCopy = new Configuration(conf);
+    // Lower the timeout so we quickly fail to connect
+    confCopy.setInt(CommonConfigurationKeysPublic.IPC_CLIENT_CONNECT_MAX_RETRIES_KEY, 1);
+    SocketFactory factory = NetUtils.getDefaultSocketFactory(confCopy);
+    return new HAServiceProtocolClientSideTranslatorPB(
+        getAddress(),
+        confCopy, factory, timeoutMs);
+  }
+
+  /**
+   * @return a proxy to connect to the target HA Service.
+   */
+  public final HAServiceProtocol getProxy() throws IOException {
+    return getProxy(new Configuration(), 0); // default conf, timeout
+  }
+}

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/NodeFencer.java

@@ -17,7 +17,6 @@
  */
 package org.apache.hadoop.ha;
 
-import java.net.InetSocketAddress;
 import java.util.List;
 import java.util.Map;
 import java.util.regex.Matcher;
@@ -91,14 +90,14 @@ public class NodeFencer {
     return new NodeFencer(conf);
   }
 
-  public boolean fence(InetSocketAddress serviceAddr) {
+  public boolean fence(HAServiceTarget fromSvc) {
     LOG.info("====== Beginning Service Fencing Process... ======");
     int i = 0;
     for (FenceMethodWithArg method : methods) {
       LOG.info("Trying method " + (++i) + "/" + methods.size() +": " + method);
       
       try {
-        if (method.method.tryFence(serviceAddr, method.arg)) {
+        if (method.method.tryFence(fromSvc, method.arg)) {
           LOG.info("====== Fencing successful by method " + method + " ======");
           return true;
         }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/ShellCommandFencer.java

@@ -75,7 +75,8 @@ public class ShellCommandFencer
   }
 
   @Override
-  public boolean tryFence(InetSocketAddress serviceAddr, String cmd) {
+  public boolean tryFence(HAServiceTarget target, String cmd) {
+    InetSocketAddress serviceAddr = target.getAddress();
     List<String> cmdList = Arrays.asList(cmd.split("\\s+"));
 
     // Create arg list with service as the first argument

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/SshFenceByTcpPort.java

@@ -79,10 +79,11 @@ public class SshFenceByTcpPort extends Configured
   }
 
   @Override
-  public boolean tryFence(InetSocketAddress serviceAddr, String argsStr)
+  public boolean tryFence(HAServiceTarget target, String argsStr)
       throws BadFencingConfigurationException {
 
     Args args = new Args(argsStr);
+    InetSocketAddress serviceAddr = target.getAddress();
     String host = serviceAddr.getHostName();
     
     Session session;

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/DummyHAService.java

@@ -0,0 +1,94 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ha;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.ha.HAServiceProtocol.HAServiceState;
+import org.apache.hadoop.security.AccessControlException;
+import org.mockito.Mockito;
+
+/**
+ * Test-only implementation of {@link HAServiceTarget}, which returns
+ * a mock implementation.
+ */
+class DummyHAService extends HAServiceTarget {
+  HAServiceState state;
+  HAServiceProtocol proxy;
+  NodeFencer fencer;
+  InetSocketAddress address;
+
+  DummyHAService(HAServiceState state, InetSocketAddress address) {
+    this.state = state;
+    this.proxy = makeMock();
+    this.fencer = Mockito.mock(NodeFencer.class);
+    this.address = address;
+  }
+  
+  private HAServiceProtocol makeMock() {
+    return Mockito.spy(new HAServiceProtocol() {
+      @Override
+      public void monitorHealth() throws HealthCheckFailedException,
+          AccessControlException, IOException {
+      }
+
+      @Override
+      public void transitionToActive() throws ServiceFailedException,
+          AccessControlException, IOException {
+        state = HAServiceState.ACTIVE;
+      }
+
+      @Override
+      public void transitionToStandby() throws ServiceFailedException,
+          AccessControlException, IOException {
+        state = HAServiceState.STANDBY;
+      }
+
+      @Override
+      public HAServiceStatus getServiceStatus() throws IOException {
+        HAServiceStatus ret = new HAServiceStatus(state);
+        if (state == HAServiceState.STANDBY) {
+          ret.setReadyToBecomeActive();
+        }
+        return ret;
+      }
+    });
+  }
+
+  @Override
+  public InetSocketAddress getAddress() {
+    return address;
+  }
+
+  @Override
+  public HAServiceProtocol getProxy(Configuration conf, int timeout)
+      throws IOException {
+    return proxy;
+  }
+
+  @Override
+  public NodeFencer getFencer() {
+    return fencer;
+  }
+
+  @Override
+  public void checkFencingConfigured() throws BadFencingConfigurationException {
+  }
+}

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/TestFailoverController.java

@@ -24,124 +24,85 @@ import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
 import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
 import org.apache.hadoop.ha.HAServiceProtocol.HAServiceState;
-import org.apache.hadoop.ha.protocolPB.HAServiceProtocolClientSideTranslatorPB;
 import org.apache.hadoop.ha.TestNodeFencer.AlwaysSucceedFencer;
 import org.apache.hadoop.ha.TestNodeFencer.AlwaysFailFencer;
 import static org.apache.hadoop.ha.TestNodeFencer.setupFencer;
-import org.apache.hadoop.net.NetUtils;
 import org.apache.hadoop.security.AccessControlException;
 
 import org.junit.Test;
+import org.mockito.Mockito;
+import org.mockito.internal.stubbing.answers.ThrowsException;
+import org.mockito.stubbing.Answer;
+
 import static org.junit.Assert.*;
 
 public class TestFailoverController {
-
   private InetSocketAddress svc1Addr = new InetSocketAddress("svc1", 1234); 
-  private InetSocketAddress svc2Addr = new InetSocketAddress("svc2", 5678); 
+  private InetSocketAddress svc2Addr = new InetSocketAddress("svc2", 5678);
 
-  private class DummyService implements HAServiceProtocol {
+  HAServiceStatus STATE_NOT_READY = new HAServiceStatus(HAServiceState.STANDBY)
-    HAServiceState state;
+      .setNotReadyToBecomeActive("injected not ready");
 
-    DummyService(HAServiceState state) {
-      this.state = state;
-    }
-
-    @Override
-    public void monitorHealth() throws HealthCheckFailedException, IOException {
-      // Do nothing
-    }
-
-    @Override
-    public void transitionToActive() throws ServiceFailedException, IOException {
-      state = HAServiceState.ACTIVE;
-    }
-
-    @Override
-    public void transitionToStandby() throws ServiceFailedException, IOException {
-      state = HAServiceState.STANDBY;
-    }
-
-    @Override
-    public HAServiceStatus getServiceStatus() throws IOException {
-      HAServiceStatus ret = new HAServiceStatus(state);
-      if (state == HAServiceState.STANDBY) {
-        ret.setReadyToBecomeActive();
-      }
-      return ret;
-    }
-    
-    private HAServiceState getServiceState() {
-      return state;
-    }
-  }
-  
   @Test
   public void testFailoverAndFailback() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY);
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     AlwaysSucceedFencer.fenceCalled = 0;
-    FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+    FailoverController.failover(svc1, svc2, false, false);
     assertEquals(0, TestNodeFencer.AlwaysSucceedFencer.fenceCalled);
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
-    assertEquals(HAServiceState.ACTIVE, svc2.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc2.state);
 
     AlwaysSucceedFencer.fenceCalled = 0;
-    FailoverController.failover(svc2, svc2Addr, svc1, svc1Addr, fencer, false, false);
+    FailoverController.failover(svc2, svc1, false, false);
     assertEquals(0, TestNodeFencer.AlwaysSucceedFencer.fenceCalled);
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
 
   @Test
   public void testFailoverFromStandbyToStandby() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.STANDBY);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.STANDBY, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY);
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
-    FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+    FailoverController.failover(svc1, svc2, false, false);
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
-    assertEquals(HAServiceState.ACTIVE, svc2.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc2.state);
   }
 
   @Test
   public void testFailoverFromActiveToActive() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc2 = new DummyHAService(HAServiceState.ACTIVE, svc2Addr);
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Can't failover to an already active service");
     } catch (FailoverFailedException ffe) {
       // Expected
     }
 
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.ACTIVE, svc2.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc2.state);
   }
 
   @Test
   public void testFailoverWithoutPermission() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE) {
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-      @Override
+    Mockito.doThrow(new AccessControlException("Access denied"))
-      public HAServiceStatus getServiceStatus() throws IOException {
+        .when(svc1.proxy).getServiceStatus();
-        throw new AccessControlException("Access denied");
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      }
+    Mockito.doThrow(new AccessControlException("Access denied"))
-    };
+        .when(svc2.proxy).getServiceStatus();
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-      @Override
-      public HAServiceStatus getServiceStatus() throws IOException {
-        throw new AccessControlException("Access denied");
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Can't failover when access is denied");
     } catch (FailoverFailedException ffe) {
       assertTrue(ffe.getCause().getMessage().contains("Access denied"));
@@ -151,19 +112,13 @@ public class TestFailoverController {
 
   @Test
   public void testFailoverToUnreadyService() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      @Override
+    Mockito.doReturn(STATE_NOT_READY).when(svc2.proxy).getServiceStatus();
-      public HAServiceStatus getServiceStatus() throws IOException {
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-        HAServiceStatus ret = new HAServiceStatus(HAServiceState.STANDBY);
-        ret.setNotReadyToBecomeActive("injected not ready");
-        return ret;
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Can't failover to a service that's not ready");
     } catch (FailoverFailedException ffe) {
       // Expected
@@ -172,95 +127,88 @@ public class TestFailoverController {
       }
     }
 
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
 
     // Forcing it means we ignore readyToBecomeActive
-    FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, true);
+    FailoverController.failover(svc1, svc2, false, true);
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
-    assertEquals(HAServiceState.ACTIVE, svc2.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc2.state);
   }
 
   @Test
   public void testFailoverToUnhealthyServiceFailsAndFailsback() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      @Override
+    Mockito.doThrow(new HealthCheckFailedException("Failed!"))
-      public void monitorHealth() throws HealthCheckFailedException {
+        .when(svc2.proxy).monitorHealth();
-        throw new HealthCheckFailedException("Failed!");
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failover to unhealthy service");
     } catch (FailoverFailedException ffe) {
       // Expected
     }
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
 
   @Test
   public void testFailoverFromFaultyServiceSucceeds() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE) {
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-      @Override
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-      public void transitionToStandby() throws ServiceFailedException {
+        .when(svc1.proxy).transitionToStandby();
-        throw new ServiceFailedException("Failed!");
+
-      }
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-    };
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY);
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     AlwaysSucceedFencer.fenceCalled = 0;
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
     } catch (FailoverFailedException ffe) {
       fail("Faulty active prevented failover");
     }
 
     // svc1 still thinks it's active, that's OK, it was fenced
     assertEquals(1, AlwaysSucceedFencer.fenceCalled);
-    assertEquals("svc1:1234", AlwaysSucceedFencer.fencedSvc);
+    assertSame(svc1, AlwaysSucceedFencer.fencedSvc);
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.ACTIVE, svc2.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc2.state);
   }
 
   @Test
   public void testFailoverFromFaultyServiceFencingFailure() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE) {
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-      @Override
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-      public void transitionToStandby() throws ServiceFailedException {
+        .when(svc1.proxy).transitionToStandby();
-        throw new ServiceFailedException("Failed!");
+
-      }
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-    };
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysFailFencer.class.getName());
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY);
-    NodeFencer fencer = setupFencer(AlwaysFailFencer.class.getName());
 
     AlwaysFailFencer.fenceCalled = 0;
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failed over even though fencing failed");
     } catch (FailoverFailedException ffe) {
       // Expected
     }
 
     assertEquals(1, AlwaysFailFencer.fenceCalled);
-    assertEquals("svc1:1234", AlwaysFailFencer.fencedSvc);
+    assertSame(svc1, AlwaysFailFencer.fencedSvc);
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
 
   @Test
   public void testFencingFailureDuringFailover() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY);
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-    NodeFencer fencer = setupFencer(AlwaysFailFencer.class.getName());
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysFailFencer.class.getName());
 
     AlwaysFailFencer.fenceCalled = 0;
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, true, false);
+      FailoverController.failover(svc1, svc2, true, false);
       fail("Failed over even though fencing requested and failed");
     } catch (FailoverFailedException ffe) {
       // Expected
@@ -269,90 +217,83 @@ public class TestFailoverController {
     // If fencing was requested and it failed we don't try to make
     // svc2 active anyway, and we don't failback to svc1.
     assertEquals(1, AlwaysFailFencer.fenceCalled);
-    assertEquals("svc1:1234", AlwaysFailFencer.fencedSvc);
+    assertSame(svc1, AlwaysFailFencer.fencedSvc);
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
   
-  private HAServiceProtocol getProtocol(String target)
-      throws IOException {
-    InetSocketAddress addr = NetUtils.createSocketAddr(target);
-    Configuration conf = new Configuration();
-    // Lower the timeout so we quickly fail to connect
-    conf.setInt(CommonConfigurationKeysPublic.IPC_CLIENT_CONNECT_MAX_RETRIES_KEY, 1);
-    return new HAServiceProtocolClientSideTranslatorPB(addr, conf);
-  }
-
   @Test
   public void testFailoverFromNonExistantServiceWithFencer() throws Exception {
-    HAServiceProtocol svc1 = getProtocol("localhost:1234");
+    DummyHAService svc1 = spy(new DummyHAService(null, svc1Addr));
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY);
+    // Getting a proxy to a dead server will throw IOException on call,
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
+    // not on creation of the proxy.
+    HAServiceProtocol errorThrowingProxy = Mockito.mock(HAServiceProtocol.class,
+        new ThrowsException(new IOException("Could not connect to host")));
+    Mockito.doReturn(errorThrowingProxy).when(svc1).getProxy();
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
     } catch (FailoverFailedException ffe) {
       fail("Non-existant active prevented failover");
     }
 
     // Don't check svc1 because we can't reach it, but that's OK, it's been fenced.
-    assertEquals(HAServiceState.ACTIVE, svc2.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc2.state);
   }
 
   @Test
   public void testFailoverToNonExistantServiceFails() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    HAServiceProtocol svc2 = getProtocol("localhost:1234");
+    DummyHAService svc2 = spy(new DummyHAService(null, svc2Addr));
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
+    Mockito.doThrow(new IOException("Failed to connect"))
+      .when(svc2).getProxy(Mockito.<Configuration>any(),
+          Mockito.anyInt());
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failed over to a non-existant standby");
     } catch (FailoverFailedException ffe) {
       // Expected
     }
 
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
   }
 
   @Test
   public void testFailoverToFaultyServiceFailsbackOK() throws Exception {
-    DummyService svc1 = spy(new DummyService(HAServiceState.ACTIVE));
+    DummyHAService svc1 = spy(new DummyHAService(HAServiceState.ACTIVE, svc1Addr));
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      @Override
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-      public void transitionToActive() throws ServiceFailedException {
+        .when(svc2.proxy).transitionToActive();
-        throw new ServiceFailedException("Failed!");
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failover to already active service");
     } catch (FailoverFailedException ffe) {
       // Expected
     }
 
     // svc1 went standby then back to active
-    verify(svc1).transitionToStandby();
+    verify(svc1.proxy).transitionToStandby();
-    verify(svc1).transitionToActive();
+    verify(svc1.proxy).transitionToActive();
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
 
   @Test
   public void testWeDontFailbackIfActiveWasFenced() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      @Override
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-      public void transitionToActive() throws ServiceFailedException {
+        .when(svc2.proxy).transitionToActive();
-        throw new ServiceFailedException("Failed!");
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, true, false);
+      FailoverController.failover(svc1, svc2, true, false);
       fail("Failed over to service that won't transition to active");
     } catch (FailoverFailedException ffe) {
       // Expected
@@ -360,24 +301,21 @@ public class TestFailoverController {
 
     // We failed to failover and did not failback because we fenced
     // svc1 (we forced it), therefore svc1 and svc2 should be standby.
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
 
   @Test
   public void testWeFenceOnFailbackIfTransitionToActiveFails() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      @Override
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-      public void transitionToActive() throws ServiceFailedException, IOException {
+        .when(svc2.proxy).transitionToActive();
-        throw new IOException("Failed!");
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
     AlwaysSucceedFencer.fenceCalled = 0;
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failed over to service that won't transition to active");
     } catch (FailoverFailedException ffe) {
       // Expected
@@ -386,25 +324,22 @@ public class TestFailoverController {
     // We failed to failover. We did not fence svc1 because it cooperated
     // and we didn't force it, so we failed back to svc1 and fenced svc2.
     // Note svc2 still thinks it's active, that's OK, we fenced it.
-    assertEquals(HAServiceState.ACTIVE, svc1.getServiceState());
+    assertEquals(HAServiceState.ACTIVE, svc1.state);
     assertEquals(1, AlwaysSucceedFencer.fenceCalled);
-    assertEquals("svc2:5678", AlwaysSucceedFencer.fencedSvc);
+    assertSame(svc2, AlwaysSucceedFencer.fencedSvc);
   }
 
   @Test
   public void testFailureToFenceOnFailbackFailsTheFailback() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE);
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      @Override
+    Mockito.doThrow(new IOException("Failed!"))
-      public void transitionToActive() throws ServiceFailedException, IOException {
+        .when(svc2.proxy).transitionToActive();
-        throw new IOException("Failed!");
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysFailFencer.class.getName());
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysFailFencer.class.getName());
     AlwaysFailFencer.fenceCalled = 0;
 
     try {
-      FailoverController.failover(svc1,  svc1Addr,  svc2,  svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failed over to service that won't transition to active");
     } catch (FailoverFailedException ffe) {
       // Expected
@@ -413,35 +348,30 @@ public class TestFailoverController {
     // We did not fence svc1 because it cooperated and we didn't force it, 
     // we failed to failover so we fenced svc2, we failed to fence svc2
     // so we did not failback to svc1, ie it's still standby.
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
     assertEquals(1, AlwaysFailFencer.fenceCalled);
-    assertEquals("svc2:5678", AlwaysFailFencer.fencedSvc);
+    assertSame(svc2, AlwaysFailFencer.fencedSvc);
   }
 
   @Test
   public void testFailbackToFaultyServiceFails() throws Exception {
-    DummyService svc1 = new DummyService(HAServiceState.ACTIVE) {
+    DummyHAService svc1 = new DummyHAService(HAServiceState.ACTIVE, svc1Addr);
-      @Override
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-      public void transitionToActive() throws ServiceFailedException {
+        .when(svc1.proxy).transitionToActive();
-        throw new ServiceFailedException("Failed!");
+    DummyHAService svc2 = new DummyHAService(HAServiceState.STANDBY, svc2Addr);
-      }
+    Mockito.doThrow(new ServiceFailedException("Failed!"))
-    };
+        .when(svc2.proxy).transitionToActive();
-    DummyService svc2 = new DummyService(HAServiceState.STANDBY) {
+
-      @Override
+    svc1.fencer = svc2.fencer = setupFencer(AlwaysSucceedFencer.class.getName());
-      public void transitionToActive() throws ServiceFailedException {
-        throw new ServiceFailedException("Failed!");
-      }
-    };
-    NodeFencer fencer = setupFencer(AlwaysSucceedFencer.class.getName());
 
     try {
-      FailoverController.failover(svc1, svc1Addr, svc2, svc2Addr, fencer, false, false);
+      FailoverController.failover(svc1, svc2, false, false);
       fail("Failover to already active service");
     } catch (FailoverFailedException ffe) {
       // Expected
     }
 
-    assertEquals(HAServiceState.STANDBY, svc1.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc1.state);
-    assertEquals(HAServiceState.STANDBY, svc2.getServiceState());
+    assertEquals(HAServiceState.STANDBY, svc2.state);
   }
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/TestHAAdmin.java

@@ -22,14 +22,15 @@ import static org.junit.Assert.*;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.PrintStream;
+import java.net.InetSocketAddress;
 
 import org.apache.commons.logging.LogFactory;
 import org.apache.commons.logging.Log;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.ha.HAServiceProtocol.HAServiceState;
 
 import org.junit.Before;
 import org.junit.Test;
-import org.mockito.Mockito;
 
 import com.google.common.base.Charsets;
 import com.google.common.base.Joiner;
@@ -40,15 +41,14 @@ public class TestHAAdmin {
   private HAAdmin tool;
   private ByteArrayOutputStream errOutBytes = new ByteArrayOutputStream();
   private String errOutput;
-  private HAServiceProtocol mockProtocol;
+
-  
   @Before
   public void setup() throws IOException {
-    mockProtocol = Mockito.mock(HAServiceProtocol.class);
     tool = new HAAdmin() {
       @Override
-      protected HAServiceProtocol getProtocol(String target) throws IOException {
+      protected HAServiceTarget resolveTarget(String target) {
-        return mockProtocol;
+        return new DummyHAService(HAServiceState.STANDBY,
+            new InetSocketAddress("dummy", 12345));
       }
     };
     tool.setConf(new Configuration());

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/TestNodeFencer.java

@@ -26,26 +26,35 @@ import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.conf.Configured;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 import com.google.common.collect.Lists;
 
 public class TestNodeFencer {
 
+  private HAServiceTarget MOCK_TARGET;
+  
+
   @Before
   public void clearMockState() {
     AlwaysSucceedFencer.fenceCalled = 0;
     AlwaysSucceedFencer.callArgs.clear();
     AlwaysFailFencer.fenceCalled = 0;
     AlwaysFailFencer.callArgs.clear();
+    
+    MOCK_TARGET = Mockito.mock(HAServiceTarget.class);
+    Mockito.doReturn("my mock").when(MOCK_TARGET).toString();
+    Mockito.doReturn(new InetSocketAddress("host", 1234))
+        .when(MOCK_TARGET).getAddress();
   }
 
   @Test
   public void testSingleFencer() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer(
         AlwaysSucceedFencer.class.getName() + "(foo)");
-    assertTrue(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertTrue(fencer.fence(MOCK_TARGET));
     assertEquals(1, AlwaysSucceedFencer.fenceCalled);
-    assertEquals("host:1234", AlwaysSucceedFencer.fencedSvc);
+    assertSame(MOCK_TARGET, AlwaysSucceedFencer.fencedSvc);
     assertEquals("foo", AlwaysSucceedFencer.callArgs.get(0));
   }
   
@@ -54,7 +63,7 @@ public class TestNodeFencer {
     NodeFencer fencer = setupFencer(
         AlwaysSucceedFencer.class.getName() + "(foo)\n" +
         AlwaysSucceedFencer.class.getName() + "(bar)\n");
-    assertTrue(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertTrue(fencer.fence(MOCK_TARGET));
     // Only one call, since the first fencer succeeds
     assertEquals(1, AlwaysSucceedFencer.fenceCalled);
     assertEquals("foo", AlwaysSucceedFencer.callArgs.get(0));
@@ -68,12 +77,12 @@ public class TestNodeFencer {
         " # the next one will always fail\n" +
         " " + AlwaysFailFencer.class.getName() + "(foo) # <- fails\n" +
         AlwaysSucceedFencer.class.getName() + "(bar) \n");
-    assertTrue(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertTrue(fencer.fence(MOCK_TARGET));
     // One call to each, since top fencer fails
     assertEquals(1, AlwaysFailFencer.fenceCalled);
-    assertEquals("host:1234", AlwaysFailFencer.fencedSvc);
+    assertSame(MOCK_TARGET, AlwaysFailFencer.fencedSvc);
     assertEquals(1, AlwaysSucceedFencer.fenceCalled);
-    assertEquals("host:1234", AlwaysSucceedFencer.fencedSvc);
+    assertSame(MOCK_TARGET, AlwaysSucceedFencer.fencedSvc);
     assertEquals("foo", AlwaysFailFencer.callArgs.get(0));
     assertEquals("bar", AlwaysSucceedFencer.callArgs.get(0));
   }
@@ -82,41 +91,41 @@ public class TestNodeFencer {
   public void testArglessFencer() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer(
         AlwaysSucceedFencer.class.getName());
-    assertTrue(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertTrue(fencer.fence(MOCK_TARGET));
     // One call to each, since top fencer fails
     assertEquals(1, AlwaysSucceedFencer.fenceCalled);
-    assertEquals("host:1234", AlwaysSucceedFencer.fencedSvc);
+    assertSame(MOCK_TARGET, AlwaysSucceedFencer.fencedSvc);
     assertEquals(null, AlwaysSucceedFencer.callArgs.get(0));
   }
 
   @Test
   public void testShortNameShell() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer("shell(true)");
-    assertTrue(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertTrue(fencer.fence(MOCK_TARGET));
   }
 
   @Test
   public void testShortNameSsh() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer("sshfence");
-    assertFalse(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertFalse(fencer.fence(MOCK_TARGET));
   }
 
   @Test
   public void testShortNameSshWithUser() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer("sshfence(user)");
-    assertFalse(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertFalse(fencer.fence(MOCK_TARGET));
   }
 
   @Test
   public void testShortNameSshWithPort() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer("sshfence(:123)");
-    assertFalse(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertFalse(fencer.fence(MOCK_TARGET));
   }
 
   @Test
   public void testShortNameSshWithUserPort() throws BadFencingConfigurationException {
     NodeFencer fencer = setupFencer("sshfence(user:123)");
-    assertFalse(fencer.fence(new InetSocketAddress("host", 1234)));
+    assertFalse(fencer.fence(MOCK_TARGET));
   }
 
   public static NodeFencer setupFencer(String confStr)
@@ -133,12 +142,12 @@ public class TestNodeFencer {
   public static class AlwaysSucceedFencer extends Configured
       implements FenceMethod {
     static int fenceCalled = 0;
-    static String fencedSvc;
+    static HAServiceTarget fencedSvc;
     static List<String> callArgs = Lists.newArrayList();
 
     @Override
-    public boolean tryFence(InetSocketAddress serviceAddr, String args) {
+    public boolean tryFence(HAServiceTarget target, String args) {
-      fencedSvc = serviceAddr.getHostName() + ":" + serviceAddr.getPort();
+      fencedSvc = target;
       callArgs.add(args);
       fenceCalled++;
       return true;
@@ -155,12 +164,12 @@ public class TestNodeFencer {
   public static class AlwaysFailFencer extends Configured
       implements FenceMethod {
     static int fenceCalled = 0;
-    static String fencedSvc;
+    static HAServiceTarget fencedSvc;
     static List<String> callArgs = Lists.newArrayList();
 
     @Override
-    public boolean tryFence(InetSocketAddress serviceAddr, String args) {
+    public boolean tryFence(HAServiceTarget target, String args) {
-      fencedSvc = serviceAddr.getHostName() + ":" + serviceAddr.getPort();
+      fencedSvc = target;
       callArgs.add(args);
       fenceCalled++;
       return false;

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/TestShellCommandFencer.java

@@ -22,6 +22,7 @@ import static org.junit.Assert.*;
 import java.net.InetSocketAddress;
 
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.ha.HAServiceProtocol.HAServiceState;
 import org.apache.hadoop.util.StringUtils;
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -32,6 +33,9 @@ import static org.mockito.Mockito.spy;
 
 public class TestShellCommandFencer {
   private ShellCommandFencer fencer = createFencer();
+  private static final HAServiceTarget TEST_TARGET =
+      new DummyHAService(HAServiceState.ACTIVE,
+          new InetSocketAddress("host", 1234));
   
   @BeforeClass
   public static void setupLogSpy() {
@@ -57,11 +61,10 @@ public class TestShellCommandFencer {
    */
   @Test
   public void testBasicSuccessFailure() {
-    InetSocketAddress addr = new InetSocketAddress("host", 1234);
+    assertTrue(fencer.tryFence(TEST_TARGET, "echo"));
-    assertTrue(fencer.tryFence(addr, "echo"));
+    assertFalse(fencer.tryFence(TEST_TARGET, "exit 1"));
-    assertFalse(fencer.tryFence(addr, "exit 1"));
     // bad path should also fail
-    assertFalse(fencer.tryFence(addr, "xxxxxxxxxxxx"));
+    assertFalse(fencer.tryFence(TEST_TARGET, "xxxxxxxxxxxx"));
   }
   
   @Test
@@ -98,8 +101,7 @@ public class TestShellCommandFencer {
    */
   @Test
   public void testStdoutLogging() {
-    InetSocketAddress addr = new InetSocketAddress("host", 1234);
+    assertTrue(fencer.tryFence(TEST_TARGET, "echo hello"));
-    assertTrue(fencer.tryFence(addr, "echo hello"));
     Mockito.verify(ShellCommandFencer.LOG).info(
         Mockito.endsWith("echo hello: host:1234 hello"));
   }
@@ -110,8 +112,7 @@ public class TestShellCommandFencer {
    */
   @Test
   public void testStderrLogging() {
-    InetSocketAddress addr = new InetSocketAddress("host", 1234);
+    assertTrue(fencer.tryFence(TEST_TARGET, "echo hello >&2"));
-    assertTrue(fencer.tryFence(addr, "echo hello >&2"));
     Mockito.verify(ShellCommandFencer.LOG).warn(
         Mockito.endsWith("echo hello >&2: host:1234 hello"));
   }
@@ -122,8 +123,7 @@ public class TestShellCommandFencer {
    */
   @Test
   public void testConfAsEnvironment() {
-    InetSocketAddress addr = new InetSocketAddress("host", 1234);
+    fencer.tryFence(TEST_TARGET, "echo $in_fencing_tests");
-    fencer.tryFence(addr, "echo $in_fencing_tests");
     Mockito.verify(ShellCommandFencer.LOG).info(
         Mockito.endsWith("echo $in...ing_tests: host:1234 yessir"));
   }
@@ -136,8 +136,7 @@ public class TestShellCommandFencer {
    */
   @Test(timeout=10000)
   public void testSubprocessInputIsClosed() {
-    InetSocketAddress addr = new InetSocketAddress("host", 1234);
+    assertFalse(fencer.tryFence(TEST_TARGET, "read"));
-    assertFalse(fencer.tryFence(addr, "read"));
   }
   
   @Test

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/TestSshFenceByTcpPort.java

@@ -23,6 +23,7 @@ import java.net.InetSocketAddress;
 
 import org.apache.commons.logging.impl.Log4JLogger;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.ha.HAServiceProtocol.HAServiceState;
 import org.apache.hadoop.ha.SshFenceByTcpPort.Args;
 import org.apache.log4j.Level;
 import org.junit.Assume;
@@ -34,12 +35,25 @@ public class TestSshFenceByTcpPort {
     ((Log4JLogger)SshFenceByTcpPort.LOG).getLogger().setLevel(Level.ALL);
   }
   
-  private String TEST_FENCING_HOST = System.getProperty(
+  private static String TEST_FENCING_HOST = System.getProperty(
       "test.TestSshFenceByTcpPort.host", "localhost");
-  private String TEST_FENCING_PORT = System.getProperty(
+  private static final String TEST_FENCING_PORT = System.getProperty(
       "test.TestSshFenceByTcpPort.port", "8020");
-  private final String TEST_KEYFILE = System.getProperty(
+  private static final String TEST_KEYFILE = System.getProperty(
       "test.TestSshFenceByTcpPort.key");
+  
+  private static final InetSocketAddress TEST_ADDR =
+    new InetSocketAddress(TEST_FENCING_HOST,
+      Integer.valueOf(TEST_FENCING_PORT));
+  private static final HAServiceTarget TEST_TARGET =
+    new DummyHAService(HAServiceState.ACTIVE, TEST_ADDR);
+  
+  /**
+   *  Connect to Google's DNS server - not running ssh!
+   */
+  private static final HAServiceTarget UNFENCEABLE_TARGET =
+    new DummyHAService(HAServiceState.ACTIVE,
+        new InetSocketAddress("8.8.8.8", 1234));
 
   @Test(timeout=20000)
   public void testFence() throws BadFencingConfigurationException {
@@ -49,8 +63,7 @@ public class TestSshFenceByTcpPort {
     SshFenceByTcpPort fence = new SshFenceByTcpPort();
     fence.setConf(conf);
     assertTrue(fence.tryFence(
-        new InetSocketAddress(TEST_FENCING_HOST,
+        TEST_TARGET,
-                              Integer.valueOf(TEST_FENCING_PORT)),
         null));
   }
 
@@ -65,8 +78,7 @@ public class TestSshFenceByTcpPort {
     conf.setInt(SshFenceByTcpPort.CONF_CONNECT_TIMEOUT_KEY, 3000);
     SshFenceByTcpPort fence = new SshFenceByTcpPort();
     fence.setConf(conf);
-    // Connect to Google's DNS server - not running ssh!
+    assertFalse(fence.tryFence(UNFENCEABLE_TARGET, ""));
-    assertFalse(fence.tryFence(new InetSocketAddress("8.8.8.8", 1234), ""));
   }
   
   @Test

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSHAAdmin.java

@@ -25,8 +25,8 @@ import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.CommonConfigurationKeys;
 import org.apache.hadoop.ha.HAAdmin;
+import org.apache.hadoop.ha.HAServiceTarget;
 import org.apache.hadoop.hdfs.DFSConfigKeys;
-import org.apache.hadoop.hdfs.DFSUtil;
 import org.apache.hadoop.hdfs.HdfsConfiguration;
 import org.apache.hadoop.util.ToolRunner;
 
@@ -65,15 +65,9 @@ public class DFSHAAdmin extends HAAdmin {
    * Try to map the given namenode ID to its service address.
    */
   @Override
-  protected String getServiceAddr(String nnId) {
+  protected HAServiceTarget resolveTarget(String nnId) {
     HdfsConfiguration conf = (HdfsConfiguration)getConf();
-    String serviceAddr = 
+    return new NNHAServiceTarget(conf, nameserviceId, nnId);
-      DFSUtil.getNamenodeServiceAddr(conf, nameserviceId, nnId);
-    if (serviceAddr == null) {
-      throw new IllegalArgumentException(
-          "Unable to determine service address for namenode '" + nnId + "'");
-    }
-    return serviceAddr;
   }
 
   @Override

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/NNHAServiceTarget.java

@@ -0,0 +1,84 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdfs.tools;
+
+import java.net.InetSocketAddress;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.ha.BadFencingConfigurationException;
+import org.apache.hadoop.ha.HAServiceTarget;
+import org.apache.hadoop.ha.NodeFencer;
+import org.apache.hadoop.hdfs.DFSUtil;
+import org.apache.hadoop.hdfs.HdfsConfiguration;
+import org.apache.hadoop.hdfs.server.namenode.NameNode;
+import org.apache.hadoop.net.NetUtils;
+
+/**
+ * One of the NN NameNodes acting as the target of an administrative command
+ * (e.g. failover).
+ */
+@InterfaceAudience.Private
+public class NNHAServiceTarget extends HAServiceTarget {
+
+  private final InetSocketAddress addr;
+  private NodeFencer fencer;
+  private BadFencingConfigurationException fenceConfigError;
+
+  public NNHAServiceTarget(HdfsConfiguration conf,
+      String nsId, String nnId) {
+    String serviceAddr = 
+      DFSUtil.getNamenodeServiceAddr(conf, nsId, nnId);
+    if (serviceAddr == null) {
+      throw new IllegalArgumentException(
+          "Unable to determine service address for namenode '" + nnId + "'");
+    }
+    this.addr = NetUtils.createSocketAddr(serviceAddr,
+        NameNode.DEFAULT_PORT);
+    try {
+      this.fencer = NodeFencer.create(conf);
+    } catch (BadFencingConfigurationException e) {
+      this.fenceConfigError = e;
+    }
+  }
+
+  /**
+   * @return the NN's IPC address.
+   */
+  @Override
+  public InetSocketAddress getAddress() {
+    return addr;
+  }
+
+  @Override
+  public void checkFencingConfigured() throws BadFencingConfigurationException {
+    if (fenceConfigError != null) {
+      throw fenceConfigError;
+    }
+  }
+  
+  @Override
+  public NodeFencer getFencer() {
+    return fencer;
+  }
+  
+  @Override
+  public String toString() {
+    return "NameNode at " + addr;
+  }
+
+}

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java

@@ -26,12 +26,14 @@ import java.io.PrintStream;
 
 import org.apache.commons.logging.LogFactory;
 import org.apache.commons.logging.Log;
+import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hdfs.DFSConfigKeys;
 import org.apache.hadoop.hdfs.DFSUtil;
 import org.apache.hadoop.hdfs.HdfsConfiguration;
 import org.apache.hadoop.ha.HAServiceProtocol;
 import org.apache.hadoop.ha.HAServiceProtocol.HAServiceState;
 import org.apache.hadoop.ha.HAServiceStatus;
+import org.apache.hadoop.ha.HAServiceTarget;
 import org.apache.hadoop.ha.HealthCheckFailedException;
 import org.apache.hadoop.ha.NodeFencer;
 
@@ -79,10 +81,19 @@ public class TestDFSHAAdmin {
   public void setup() throws IOException {
     mockProtocol = Mockito.mock(HAServiceProtocol.class);
     tool = new DFSHAAdmin() {
+
       @Override
-      protected HAServiceProtocol getProtocol(String serviceId) throws IOException {
+      protected HAServiceTarget resolveTarget(String nnId) {
-        getServiceAddr(serviceId);
+        HAServiceTarget target = super.resolveTarget(nnId);
-        return mockProtocol;
+        HAServiceTarget spy = Mockito.spy(target);
+        // OVerride the target to return our mock protocol
+        try {
+          Mockito.doReturn(mockProtocol).when(spy).getProxy(
+              Mockito.<Configuration>any(), Mockito.anyInt()); 
+        } catch (IOException e) {
+          throw new AssertionError(e); // mock setup doesn't really throw
+        }
+        return spy;
       }
     };
     tool.setConf(getHAConf());

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdminMiniCluster.java

@@ -25,12 +25,15 @@ import java.io.PrintStream;
 
 import org.apache.commons.logging.LogFactory;
 import org.apache.commons.logging.Log;
+import org.apache.commons.logging.impl.Log4JLogger;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hdfs.MiniDFSCluster;
 import org.apache.hadoop.hdfs.MiniDFSNNTopology;
 import org.apache.hadoop.hdfs.server.namenode.NameNode;
 import org.apache.hadoop.hdfs.server.namenode.NameNodeAdapter;
+import org.apache.hadoop.ha.HAAdmin;
 import org.apache.hadoop.ha.NodeFencer;
+import org.apache.log4j.Level;
 
 import org.junit.After;
 import org.junit.Before;
@@ -43,6 +46,10 @@ import com.google.common.base.Joiner;
  * Tests for HAAdmin command with {@link MiniDFSCluster} set up in HA mode.
  */
 public class TestDFSHAAdminMiniCluster {
+  static {
+    ((Log4JLogger)LogFactory.getLog(HAAdmin.class)).getLogger().setLevel(
+        Level.ALL);
+  }
   private static final Log LOG = LogFactory.getLog(TestDFSHAAdminMiniCluster.class);
   
   private MiniDFSCluster cluster;