[
https://issues.apache.org/jira/browse/HBASE-19239?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16254266#comment-16254266
]
Andrew Purtell edited comment on HBASE-19239 at 11/15/17 10:17 PM:
-------------------------------------------------------------------
I found a scary bug in the assignment manager, where we are relying on
reference equality to avoid making a mistake with planning, and getting lucky
because equals and hashCode for RegionPlan are IMHO broken. Here's the patch
which fixes both problems. I'm going to run the full suite now but wanted
comment on this change.
{code}
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
index 196ef05beb..b8c088a583 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
@@ -2360,7 +2360,8 @@ public class AssignmentManager extends ZooKeeperListener {
return;
}
- if (plan != newPlan &&
!plan.getDestination().equals(newPlan.getDestination())) {
+ if (!plan.equals(newPlan) &&
+ !plan.getDestination().equals(newPlan.getDestination())) {
// Clean out plan we failed execute and one that doesn't look like
it'll
// succeed anyways; we need a new plan!
// Transition back to OFFLINE
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
index cd6b3131e1..92b08b8da4 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
@@ -118,19 +118,30 @@ public class RegionPlan implements Comparable<RegionPlan>
{
@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
public boolean equals(Object obj) {
- if (this == obj) {
- return true;
- }
- if (obj == null || getClass() != obj.getClass()) {
- return false;
- }
+ if (this == obj) return true;
+ 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
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRegionPlan.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRegionPlan.java
index e5b1ca5fac..545f9bbc23 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRegionPlan.java
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRegionPlan.java
@@ -35,14 +35,13 @@ public class TestRegionPlan {
ServerName source = ServerName.valueOf("source", 1234, 2345);
ServerName dest = ServerName.valueOf("dest", 1234, 2345);
- // Identiy equality
+ // Identity equality
RegionPlan plan = new RegionPlan(hri, source, dest);
assertEquals(plan.hashCode(), new RegionPlan(hri, source,
dest).hashCode());
assertEquals(plan, new RegionPlan(hri, source, dest));
- // Source and destination not used for equality
- assertEquals(plan.hashCode(), new RegionPlan(hri, dest,
source).hashCode());
- assertEquals(plan, new RegionPlan(hri, dest, source));
+ // Source and destination not used for comparison
+ assertEquals(0, plan.compareTo(new RegionPlan(hri, dest, source)));
// HRI is used for equality
HRegionInfo other = new HRegionInfo(TableName.valueOf("other"));
{code}
Same issues exist in branch-2.
[~stack]
was (Author: apurtell):
I found a scary bug in the assignment manager, where we are relying on
reference equality to avoid making a mistake with planning, and getting lucky
because equals and hashCode for RegionPlan are IMHO broken. Here's the patch
which fixes both problems. I'm going to run the full suite now but wanted
comment on this change.
{code}
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
index 196ef05beb..b8c088a583 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
@@ -2360,7 +2360,8 @@ public class AssignmentManager extends ZooKeeperListener {
return;
}
- if (plan != newPlan &&
!plan.getDestination().equals(newPlan.getDestination())) {
+ if (!plan.equals(newPlan) &&
+ !plan.getDestination().equals(newPlan.getDestination())) {
// Clean out plan we failed execute and one that doesn't look like
it'll
// succeed anyways; we need a new plan!
// Transition back to OFFLINE
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
index cd6b3131e1..92b08b8da4 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionPlan.java
@@ -118,19 +118,30 @@ public class RegionPlan implements Comparable<RegionPlan>
{
@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
public boolean equals(Object obj) {
- if (this == obj) {
- return true;
- }
- if (obj == null || getClass() != obj.getClass()) {
- return false;
- }
+ if (this == obj) return true;
+ 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
{code}
Same issues exist in branch-2.
[~stack]
> Fix findbugs and error-prone warnings
> -------------------------------------
>
> Key: HBASE-19239
> URL: https://issues.apache.org/jira/browse/HBASE-19239
> Project: HBase
> Issue Type: Improvement
> Reporter: Andrew Purtell
> Assignee: Andrew Purtell
> Fix For: 3.0.0, 1.4.0, 2.0.0-beta-1
>
>
> Fix important findbugs and error-prone warnings on branch-1.4 / branch-1.
> Forward port as appropriate.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
