HBASE-23753 Update of errorprone generated failures

Signed-off-by: Bharath Vissapragada <bharathv@apache.org>

hbase-client/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
 Complains about mismatch in types when Compare. Implement Compare in
 base Interface.

hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
 Complains pbs never return null.

hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSinkManager.java
 Needed redo because errorprone complains can't mock Service from guava.

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicasWithRestartScenarios.java
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
 Unrelated...adding one-liner debug statements chasing other test
 failures.

hbase-client/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java

@@ -74,7 +74,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos;
  */
 @Deprecated
 @InterfaceAudience.Public
-public class HRegionInfo implements RegionInfo, Comparable<HRegionInfo> {
+public class HRegionInfo implements RegionInfo {
   private static final Logger LOG = LoggerFactory.getLogger(HRegionInfo.class);
 
   /**
@@ -701,15 +701,6 @@ public class HRegionInfo implements RegionInfo, Comparable<HRegionInfo> {
     return this.hashCode;
   }
 
-  //
-  // Comparable
-  //
-
-  @Override
-  public int compareTo(HRegionInfo o) {
-    return RegionInfo.COMPARATOR.compare(this, o);
-  }
-
   /**
    * @return Comparator to use comparing {@link KeyValue}s.
    * @deprecated Use Region#getCellComparator().  deprecated for hbase 2.0, remove for hbase 3.0

hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java

@@ -68,7 +68,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos;
  *
  */
 @InterfaceAudience.Public
-public interface RegionInfo {
+public interface RegionInfo extends Comparable<RegionInfo> {
   RegionInfo UNDEFINED = RegionInfoBuilder.newBuilder(TableName.valueOf("__UNDEFINED__")).build();
   /**
    * Separator used to demarcate the encodedName in a region name
@@ -822,4 +822,8 @@ public interface RegionInfo {
     }
     return Bytes.compareTo(getStartKey(), other.getEndKey()) < 0;
   }
+
+  default int compareTo(RegionInfo other) {
+    return RegionInfo.COMPARATOR.compare(this, other);
+  }
 }

hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfoBuilder.java

@@ -1,5 +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
@@ -130,7 +129,7 @@ public class RegionInfoBuilder {
    * An implementation of RegionInfo that adds mutable methods so can build a RegionInfo instance.
    */
   @InterfaceAudience.Private
-  static class MutableRegionInfo implements RegionInfo, Comparable<RegionInfo> {
+  static class MutableRegionInfo implements RegionInfo {
     /**
      * The new format for a region name contains its encodedName at the end.
      * The encoded name also serves as the directory name for the region
@@ -453,7 +452,7 @@ public class RegionInfoBuilder {
       if (!(o instanceof RegionInfo)) {
         return false;
       }
-      return this.compareTo((RegionInfo)o) == 0;
+      return compareTo((RegionInfo)o) == 0;
     }
 
     /**
@@ -463,11 +462,5 @@ public class RegionInfoBuilder {
     public int hashCode() {
       return this.hashCode;
     }
-
-    @Override
-    public int compareTo(RegionInfo other) {
-      return RegionInfo.COMPARATOR.compare(this, other);
-    }
-
   }
 }

hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java

@@ -1346,7 +1346,7 @@ public final class ProtobufUtil {
     List<CellProtos.Cell> values = proto.getCellList();
 
     if (proto.hasExists()) {
-      if ((values != null && !values.isEmpty()) ||
+      if (!values.isEmpty() ||
           (proto.hasAssociatedCellCount() && proto.getAssociatedCellCount() > 0)) {
         throw new IllegalArgumentException("bad proto: exists with cells is no allowed " + proto);
       }

hbase-server/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java

@@ -1,4 +1,4 @@
-/**
+/*
  *
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
@@ -52,7 +52,7 @@ import org.slf4j.LoggerFactory;
  * @see ExecutorService
  */
 @InterfaceAudience.Private
-public abstract class EventHandler implements Runnable, Comparable<Runnable> {
+public abstract class EventHandler implements Runnable, Comparable<EventHandler> {
   private static final Logger LOG = LoggerFactory.getLogger(EventHandler.class);
 
   // type of event this object represents
@@ -152,12 +152,14 @@ public abstract class EventHandler implements Runnable, Comparable<Runnable> {
    * priority beyond FIFO, they should override {@link #getPriority()}.
    */
   @Override
-  public int compareTo(Runnable o) {
-    EventHandler eh = (EventHandler)o;
-    if(getPriority() != eh.getPriority()) {
-      return (getPriority() < eh.getPriority()) ? -1 : 1;
+  public int compareTo(EventHandler o) {
+    if (o == null) {
+      return 1;
     }
-    return (this.seqid < eh.seqid) ? -1 : 1;
+    if(getPriority() != o.getPriority()) {
+      return (getPriority() < o.getPriority()) ? -1 : 1;
+    }
+    return (this.seqid < o.seqid) ? -1 : 1;
   }
 
   @Override

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicasWithRestartScenarios.java

@@ -137,6 +137,7 @@ public class TestRegionReplicasWithRestartScenarios {
 
   private void assertReplicaDistributed() throws Exception {
     Collection<HRegion> onlineRegions = getRS().getOnlineRegionsLocalContext();
+    LOG.info("ASSERT DISTRIBUTED {}", onlineRegions);
     boolean res = checkDuplicates(onlineRegions);
     assertFalse(res);
     Collection<HRegion> onlineRegions2 = getSecondaryRS().getOnlineRegionsLocalContext();

hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSinkManager.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
@@ -19,7 +19,6 @@ package org.apache.hadoop.hbase.replication.regionserver;
 
 import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
 import java.util.List;
 import org.apache.hadoop.conf.Configuration;
@@ -45,13 +44,37 @@ public class TestReplicationSinkManager {
   public static final HBaseClassTestRule CLASS_RULE =
       HBaseClassTestRule.forClass(TestReplicationSinkManager.class);
 
-  private HBaseReplicationEndpoint replicationEndpoint;
   private ReplicationSinkManager sinkManager;
+  private HBaseReplicationEndpoint replicationEndpoint;
+
+  /**
+   * Manage the 'getRegionServers' for the tests below. Override the base class handling
+   * of Regionservers. We used to use a mock for this but updated guava/errorprone disallows
+   * mocking of classes that implement Service.
+   */
+  private static class SetServersHBaseReplicationEndpoint extends HBaseReplicationEndpoint {
+    List<ServerName> regionServers;
+
+    @Override
+    public boolean replicate(ReplicateContext replicateContext) {
+      return false;
+    }
+
+    @Override
+    public synchronized void setRegionServers(List<ServerName> regionServers) {
+      this.regionServers = regionServers;
+    }
+
+    @Override
+    public List<ServerName> getRegionServers() {
+      return this.regionServers;
+    }
+  }
 
   @Before
   public void setUp() {
-    replicationEndpoint = mock(HBaseReplicationEndpoint.class);
-    sinkManager = new ReplicationSinkManager(mock(AsyncClusterConnection.class),
+    this.replicationEndpoint = new SetServersHBaseReplicationEndpoint();
+    this.sinkManager = new ReplicationSinkManager(mock(AsyncClusterConnection.class),
       replicationEndpoint, new Configuration());
   }
 
@@ -62,12 +85,8 @@ public class TestReplicationSinkManager {
     for (int i = 0; i < totalServers; i++) {
       serverNames.add(mock(ServerName.class));
     }
-
-    when(replicationEndpoint.getRegionServers())
-          .thenReturn(serverNames);
-
+    replicationEndpoint.setRegionServers(serverNames);
     sinkManager.chooseSinks();
-
     int expected = (int) (totalServers * ReplicationSinkManager.DEFAULT_REPLICATION_SOURCE_RATIO);
     assertEquals(expected, sinkManager.getNumSinks());
 
@@ -77,12 +96,8 @@ public class TestReplicationSinkManager {
   public void testChooseSinks_LessThanRatioAvailable() {
     List<ServerName> serverNames = Lists.newArrayList(mock(ServerName.class),
       mock(ServerName.class));
-
-    when(replicationEndpoint.getRegionServers())
-          .thenReturn(serverNames);
-
+    replicationEndpoint.setRegionServers(serverNames);
     sinkManager.chooseSinks();
-
     assertEquals(1, sinkManager.getNumSinks());
   }
 
@@ -90,9 +105,7 @@ public class TestReplicationSinkManager {
   public void testReportBadSink() {
     ServerName serverNameA = mock(ServerName.class);
     ServerName serverNameB = mock(ServerName.class);
-    when(replicationEndpoint.getRegionServers())
-      .thenReturn(Lists.newArrayList(serverNameA, serverNameB));
-
+    replicationEndpoint.setRegionServers(Lists.newArrayList(serverNameA, serverNameB));
     sinkManager.chooseSinks();
     // Sanity check
     assertEquals(1, sinkManager.getNumSinks());
@@ -117,10 +130,7 @@ public class TestReplicationSinkManager {
     for (int i = 0; i < totalServers; i++) {
       serverNames.add(mock(ServerName.class));
     }
-    when(replicationEndpoint.getRegionServers())
-          .thenReturn(serverNames);
-
-
+    replicationEndpoint.setRegionServers(serverNames);
     sinkManager.chooseSinks();
     // Sanity check
     int expected = (int) (totalServers * ReplicationSinkManager.DEFAULT_REPLICATION_SOURCE_RATIO);
@@ -172,10 +182,7 @@ public class TestReplicationSinkManager {
     for (int i = 0; i < totalServers; i++) {
       serverNames.add(mock(ServerName.class));
     }
-    when(replicationEndpoint.getRegionServers())
-          .thenReturn(serverNames);
-
-
+    replicationEndpoint.setRegionServers(serverNames);
     sinkManager.chooseSinks();
     // Sanity check
     List<ServerName> sinkList = sinkManager.getSinksForTesting();

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java

@@ -652,6 +652,7 @@ public class TestSnapshotScannerHDFSAclController {
       admin.restoreSnapshot(snapshot);
       admin.snapshot(snapshot3, table);
 
+      LOG.info("CHECK");
       TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot, -1);
       TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot2, -1);
       TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot3, -1);
@@ -1085,4 +1086,4 @@ final class TestHDFSAclHelper {
       return null;
     };
   }
-}
+}