HBASE-19276 RegionPlan should correctly implement equals and hashCode

Andrew's patch that adds equals and hash but revamping compare also.

Signed-off-by: Andrew Purtell <apurtell@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java

@@ -1,4 +1,4 @@
-/**
+/*
  * 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
@@ -43,15 +43,11 @@ public class RegionPlan implements Comparable<RegionPlan> {
   private ServerName dest;
 
   public static class RegionPlanComparator implements Comparator<RegionPlan>, Serializable {
-
     private static final long serialVersionUID = 4213207330485734853L;
 
     @Override
     public int compare(RegionPlan l, RegionPlan r) {
-      long diff = r.getRegionInfo().getRegionId() - l.getRegionInfo().getRegionId();
-      if (diff < 0) return -1;
-      if (diff > 0) return 1;
-      return 0;
+      return RegionPlan.compareTo(l, r);
     }
   }
 
@@ -109,16 +105,50 @@ public class RegionPlan implements Comparable<RegionPlan> {
 
   /**
    * Compare the region info.
-   * @param o region plan you are comparing against
+   * @param other region plan you are comparing against
    */
   @Override
-  public int compareTo(RegionPlan o) {
-    return getRegionName().compareTo(o.getRegionName());
+  public int compareTo(RegionPlan other) {
+    return compareTo(this, other);
+  }
+
+  private static int compareTo(RegionPlan left, RegionPlan right) {
+    int result = compareServerName(left.source, right.source);
+    if (result != 0) {
+      return result;
+    }
+    if (left.hri == null) {
+      if (right.hri != null) {
+        return -1;
+      }
+    } else if (right.hri == null) {
+      return +1;
+    } else {
+      result = RegionInfo.COMPARATOR.compare(left.hri, right.hri);
+    }
+    if (result != 0) {
+      return result;
+    }
+    return compareServerName(left.dest, right.dest);
+  }
+
+  private static int compareServerName(ServerName left, ServerName right) {
+    if (left == null) {
+      return right == null? 0: -1;
+    } else if (right == null) {
+      return +1;
+    }
+    return left.compareTo(right);
   }
 
   @Override
   public int hashCode() {
-    return getRegionName().hashCode();
+    final int prime = 31;
+    int result = 1;
+    result = prime * result + ((dest == null) ? 0 : dest.hashCode());
+    result = prime * result + ((hri == null) ? 0 : hri.hashCode());
+    result = prime * result + ((source == null) ? 0 : source.hashCode());
+    return result;
   }
 
   @Override
@@ -126,11 +156,35 @@ public class RegionPlan implements Comparable<RegionPlan> {
     if (this == obj) {
       return true;
     }
-    if (obj == null || getClass() != obj.getClass()) {
+    if (obj == null) {
+      return false;
+    }
+    if (getClass() != obj.getClass()) {
       return false;
     }
     RegionPlan other = (RegionPlan) obj;
-    return compareTo(other) == 0;
+    if (dest == null) {
+      if (other.dest != null) {
+        return false;
+      }
+    } else if (!dest.equals(other.dest)) {
+      return false;
+    }
+    if (hri == null) {
+      if (other.hri != null) {
+        return false;
+      }
+    } else if (!hri.equals(other.hri)) {
+      return false;
+    }
+    if (source == null) {
+      if (other.source != null) {
+        return false;
+      }
+    } else if (!source.equals(other.source)) {
+      return false;
+    }
+    return true;
   }
 
   @Override

hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRegionPlan.java

@@ -1,4 +1,4 @@
-/**
+/*
  * 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
@@ -17,12 +17,15 @@
  */
 package org.apache.hadoop.hbase.master;
 
+import static junit.framework.TestCase.assertFalse;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
 
-import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.RegionInfoBuilder;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.testclassification.SmallTests;
 import org.junit.Rule;
@@ -32,27 +35,64 @@ import org.junit.rules.TestName;
 
 @Category({MasterTests.class, SmallTests.class})
 public class TestRegionPlan {
+  private final ServerName SRC = ServerName.valueOf("source", 1234, 2345);
+  private final ServerName DEST = ServerName.valueOf("dest", 1234, 2345);
   @Rule
   public TestName name = new TestName();
 
   @Test
-  public void test() {
-    HRegionInfo hri = new HRegionInfo(TableName.valueOf(name.getMethodName()));
-    ServerName source = ServerName.valueOf("source", 1234, 2345);
-    ServerName dest = ServerName.valueOf("dest", 1234, 2345);
-    
-    // Identiy equality
-    RegionPlan plan = new RegionPlan(hri, source, dest);
-    assertEquals(plan.hashCode(), new RegionPlan(hri, source, dest).hashCode());
-    assertEquals(plan, new RegionPlan(hri, source, dest));
+  public void testCompareTo() {
+    RegionInfo hri = RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName())).build();
+    RegionPlan a = new RegionPlan(hri, null, null);
+    RegionPlan b = new RegionPlan(hri, null, null);
+    assertEquals(0, a.compareTo(b));
+    a = new RegionPlan(hri, SRC, null);
+    b = new RegionPlan(hri, null, null);
+    assertEquals(1, a.compareTo(b));
+    a = new RegionPlan(hri, null, null);
+    b = new RegionPlan(hri, SRC, null);
+    assertEquals(-1, a.compareTo(b));
+    a = new RegionPlan(hri, SRC, null);
+    b = new RegionPlan(hri, SRC, null);
+    assertEquals(0, a.compareTo(b));
+    a = new RegionPlan(hri, SRC, null);
+    b = new RegionPlan(hri, SRC, DEST);
+    assertEquals(-1, a.compareTo(b));
+    a = new RegionPlan(hri, SRC, DEST);
+    b = new RegionPlan(hri, SRC, DEST);
+    assertEquals(0, a.compareTo(b));
+  }
 
-    // Source and destination not used for equality
-    assertEquals(plan.hashCode(), new RegionPlan(hri, dest, source).hashCode());
-    assertEquals(plan, new RegionPlan(hri, dest, source));
+  @Test
+  public void testEqualsWithNulls() {
+    RegionInfo hri = RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName())).build();
+    RegionPlan a = new RegionPlan(hri, null, null);
+    RegionPlan b = new RegionPlan(hri, null, null);
+    assertTrue(a.equals(b));
+    a = new RegionPlan(hri, SRC, null);
+    b = new RegionPlan(hri, null, null);
+    assertFalse(a.equals(b));
+    a = new RegionPlan(hri, SRC, null);
+    b = new RegionPlan(hri, SRC, null);
+    assertTrue(a.equals(b));
+    a = new RegionPlan(hri, SRC, null);
+    b = new RegionPlan(hri, SRC, DEST);
+    assertFalse(a.equals(b));
+  }
+
+  @Test
+  public void testEquals() {
+    RegionInfo hri = RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName())).build();
+
+    // Identity equality
+    RegionPlan plan = new RegionPlan(hri, SRC, DEST);
+    assertEquals(plan.hashCode(), new RegionPlan(hri, SRC, DEST).hashCode());
+    assertEquals(plan, new RegionPlan(hri, SRC, DEST));
 
     // HRI is used for equality
-    HRegionInfo other = new HRegionInfo(TableName.valueOf(name.getMethodName() + "other"));
-    assertNotEquals(plan.hashCode(), new RegionPlan(other, source, dest).hashCode());
-    assertNotEquals(plan, new RegionPlan(other, source, dest));
+    RegionInfo other =
+        RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName() + "other")).build();
+    assertNotEquals(plan.hashCode(), new RegionPlan(other, SRC, DEST).hashCode());
+    assertNotEquals(plan, new RegionPlan(other, SRC, DEST));
   }
 }