From b7c7b4a5702ab94156ee753823bd02342a09e3d2 Mon Sep 17 00:00:00 2001
From: stack <stack@apache.org>
Date: Mon, 19 Aug 2019 12:57:12 -0700
Subject: [PATCH] HBASE-22882
 TestFlushSnapshotFromClient#testConcurrentSnapshottingAttempts is flakey (was
 written flakey)

---
 .../hbase/procedure/ProcedureCoordinator.java   |  5 ++---
 .../snapshot/TestFlushSnapshotFromClient.java   | 17 +++++++++++------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
index 5a1516e6ce4d..d5800b12969c 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
@@ -29,6 +29,7 @@
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 
+import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -67,9 +68,9 @@ public class ProcedureCoordinator {
    * The rpc object registers the ProcedureCoordinator and starts any threads in this
    * constructor.
    *
-   * @param rpcs
    * @param pool Used for executing procedures.
    */
+  @VisibleForTesting // Only used in tests. SimpleMasterProcedureManager is a test class.
   public ProcedureCoordinator(ProcedureCoordinatorRpcs rpcs, ThreadPoolExecutor pool) {
     this(rpcs, pool, TIMEOUT_MILLIS_DEFAULT, WAKE_MILLIS_DEFAULT);
   }
@@ -80,9 +81,7 @@ public ProcedureCoordinator(ProcedureCoordinatorRpcs rpcs, ThreadPoolExecutor po
    * The rpc object registers the ProcedureCoordinator and starts any threads in
    * this constructor.
    *
-   * @param rpcs
    * @param pool Used for executing procedures.
-   * @param timeoutMillis
    */
   public ProcedureCoordinator(ProcedureCoordinatorRpcs rpcs, ThreadPoolExecutor pool,
       long timeoutMillis, long wakeTimeMillis) {
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestFlushSnapshotFromClient.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestFlushSnapshotFromClient.java
index 74ae8303f5ec..73d68f28321e 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestFlushSnapshotFromClient.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestFlushSnapshotFromClient.java
@@ -52,6 +52,7 @@
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.slf4j.Logger;
@@ -136,7 +137,6 @@ public static void cleanupTest() throws Exception {
 
   /**
    * Test simple flush snapshotting a table that is online
-   * @throws Exception
    */
   @Test
   public void testFlushTableSnapshot() throws Exception {
@@ -169,7 +169,6 @@ public void testFlushTableSnapshot() throws Exception {
 
    /**
    * Test snapshotting a table that is online without flushing
-   * @throws Exception
    */
   @Test
   public void testSkipFlushTableSnapshot() throws Exception {
@@ -209,7 +208,6 @@ public void testSkipFlushTableSnapshot() throws Exception {
 
   /**
    * Test simple flush snapshotting a table that is online
-   * @throws Exception
    */
   @Test
   public void testFlushTableSnapshotWithProcedure() throws Exception {
@@ -408,13 +406,20 @@ public void testFlushCreateListDestroy() throws Exception {
   /**
    * Demonstrate that we reject snapshot requests if there is a snapshot already running on the
    * same table currently running and that concurrent snapshots on different tables can both
-   * succeed concurretly.
+   * succeed concurrently.
    */
+  @Ignore // Turning this test off. It is written flakey (See original submission in HBASE-7536
+  // for admission). While the test ensures we run one snapshot at a time as the above comment
+  // describes, the second part of the comment where we are supposed to allow snapshots against
+  // different tables run concurrently is where there is issue. There is only one
+  // handler thread on the regionserver-side which means that on a second submission, the
+  // second request gets rejected with RejectedExecutionException. The test used to sort-of
+  // pass with 20 attempts but as often-as-not, fails.
   @Test
   public void testConcurrentSnapshottingAttempts() throws IOException, InterruptedException {
     final TableName TABLE2_NAME = TableName.valueOf(TABLE_NAME + "2");
 
-    int ssNum = 20;
+    int ssNum = 2;
     // make sure we don't fail on listing snapshots
     SnapshotTestingUtils.assertNoSnapshots(admin);
     // create second testing table
@@ -451,7 +456,7 @@ public void run() {
     // build descriptions
     SnapshotDescription[] descs = new SnapshotDescription[ssNum];
     for (int i = 0; i < ssNum; i++) {
-      if(i % 2 ==0) {
+      if (i % 2 == 0) {
         descs[i] = new SnapshotDescription("ss" + i, TABLE_NAME, SnapshotType.FLUSH);
       } else {
         descs[i] = new SnapshotDescription("ss" + i, TABLE2_NAME, SnapshotType.FLUSH);