HBASE-23731: De-flake TestFromClientSide

hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseClassTestRule.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,10 +17,16 @@
  */
 package org.apache.hadoop.hbase;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
-
 import org.apache.hadoop.hbase.testclassification.IntegrationTests;
 import org.apache.hadoop.hbase.testclassification.LargeTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
@@ -30,8 +36,14 @@
 import org.junit.rules.TestRule;
 import org.junit.rules.Timeout;
 import org.junit.runner.Description;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
 import org.junit.runners.model.Statement;
-
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
+import org.apache.hbase.thirdparty.com.google.common.collect.Iterables;
 import org.apache.hbase.thirdparty.com.google.common.collect.Sets;
 
 /**
@@ -43,9 +55,13 @@
  */
 @InterfaceAudience.Private
 public final class HBaseClassTestRule implements TestRule {
+  private static final Logger LOG = LoggerFactory.getLogger(HBaseClassTestRule.class);
   public static final Set<Class<?>> UNIT_TEST_CLASSES = Collections.unmodifiableSet(
       Sets.<Class<?>> newHashSet(SmallTests.class, MediumTests.class, LargeTests.class));
 
+  // Each unit test has this timeout.
+  private static long PER_UNIT_TEST_TIMEOUT_MINS = 13;
+
   private final Class<?> clazz;
 
   private final Timeout timeout;
@@ -65,13 +81,16 @@ public Class<?> getClazz() {
 
   private static long getTimeoutInSeconds(Class<?> clazz) {
     Category[] categories = clazz.getAnnotationsByType(Category.class);
-
+    // Starting JUnit 4.13, it appears that the timeout is applied across all the parameterized
+    // runs. So the timeout is multiplied by number of parameterized runs.
+    int numParams = getNumParameters(clazz);
     // @Category is not repeatable -- it is only possible to get an array of length zero or one.
     if (categories.length == 1) {
       for (Class<?> c : categories[0].value()) {
         if (UNIT_TEST_CLASSES.contains(c)) {
-          // All tests have a 13 minutes timeout.
-          return TimeUnit.MINUTES.toSeconds(13);
+          long timeout = numParams * PER_UNIT_TEST_TIMEOUT_MINS;
+          LOG.info("Test {} timeout: {} mins", clazz, timeout);
+          return TimeUnit.MINUTES.toSeconds(timeout);
         }
         if (c == IntegrationTests.class) {
           return TimeUnit.MINUTES.toSeconds(Long.MAX_VALUE);
@@ -82,6 +101,59 @@ private static long getTimeoutInSeconds(Class<?> clazz) {
         clazz.getName() + " does not have SmallTests/MediumTests/LargeTests in @Category");
   }
 
+  /**
+   * @param clazz Test class that is running.
+   * @return the number of parameters for this given test class. If the test is not parameterized or
+   *   if there is any issue determining the number of parameters, returns 1.
+   */
+  @VisibleForTesting
+  static int getNumParameters(Class<?> clazz) {
+    RunWith[] runWiths = clazz.getAnnotationsByType(RunWith.class);
+    boolean testParameterized = runWiths != null && Arrays.stream(runWiths).anyMatch(
+      (r) -> r.value().equals(Parameterized.class));
+    if (!testParameterized) {
+      return 1;
+    }
+    for (Method method : clazz.getMethods()) {
+      if (!isParametersMethod(method)) {
+        continue;
+      }
+      // Found the parameters method. Figure out the number of parameters.
+      Object parameters;
+      try {
+        parameters = method.invoke(clazz);
+      } catch (IllegalAccessException | InvocationTargetException e) {
+        LOG.warn("Error invoking parameters method {} in test class {}",
+            method.getName(), clazz, e);
+        continue;
+      }
+      if (parameters instanceof List) {
+        return  ((List) parameters).size();
+      } else if (parameters instanceof Collection) {
+        return  ((Collection) parameters).size();
+      } else if (parameters instanceof Iterable) {
+        return Iterables.size((Iterable) parameters);
+      } else if (parameters instanceof Object[]) {
+        return ((Object[]) parameters).length;
+      }
+    }
+    LOG.warn("Unable to determine parameters size. Returning the default of 1.");
+    return 1;
+  }
+
+  /**
+   * Helper method that checks if the input method is a valid JUnit @Parameters method.
+   * @param method Input method.
+   * @return true if the method is a valid JUnit parameters method, false otherwise.
+   */
+  private static boolean isParametersMethod(@NonNull Method method) {
+    // A valid parameters method is public static and with @Parameters annotation.
+    boolean methodPublicStatic = Modifier.isPublic(method.getModifiers()) &&
+        Modifier.isStatic(method.getModifiers());
+    Parameters[] params = method.getAnnotationsByType(Parameters.class);
+    return methodPublicStatic && (params != null && params.length > 0);
+  }
+
   public static HBaseClassTestRule forClass(Class<?> clazz) {
     return new HBaseClassTestRule(clazz, Timeout.builder().withLookingForStuckThread(true)
         .withTimeout(getTimeoutInSeconds(clazz), TimeUnit.SECONDS).build());

hbase-common/src/test/java/org/apache/hadoop/hbase/TestHBaseClassTestRule.java

@@ -0,0 +1,145 @@
+/*
+ * 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.hbase;
+
+import static junit.framework.TestCase.assertEquals;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+import org.apache.hbase.thirdparty.com.google.common.collect.Iterables;
+
+/**
+ * Tests HBaseClassTestRule.
+ */
+@Category(SmallTests.class)
+public class TestHBaseClassTestRule {
+
+  @ClassRule
+  public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(
+      TestHBaseClassTestRule.class);
+
+  // Test input classes of various kinds.
+  private static class NonParameterizedClass {
+    void dummy() {
+    }
+    int dummy(int a) {
+      return 0;
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class ParameterizedClassWithNoParametersMethod {
+    void dummy() {
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class InValidParameterizedClass {
+    // Not valid because parameters method is private.
+    @Parameters
+    private static List<Object> parameters() {
+      return Arrays.asList(1, 2, 3, 4);
+    }
+    int dummy(int a) {
+      return 0;
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class ValidParameterizedClass1 {
+    @Parameters
+    public static List<Object> parameters() {
+      return Arrays.asList(1, 2, 3, 4, 5);
+    }
+    int dummy(int a) {
+      return 0;
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class ValidParameterizedClass2 {
+    @Parameters
+    public static Object[] parameters() {
+      return new Integer[] {1, 2, 3, 4, 5, 6};
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class ValidParameterizedClass3 {
+    @Parameters
+    public static Iterable<Integer> parameters() {
+      return Arrays.asList(1, 2, 3, 4, 5, 6, 7);
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class ValidParameterizedClass4 {
+    @Parameters
+    public static Collection<Integer> parameters() {
+      return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9);
+    }
+  }
+
+
+  @RunWith(Parameterized.class)
+  private static class ExtendedParameterizedClass1 extends ValidParameterizedClass1 {
+    // Should be inferred from the parent class.
+    int dummy2(int a) {
+      return 0;
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  private static class ExtendedParameterizedClass2 extends ValidParameterizedClass1 {
+    // Should override the parent parameters class.
+    @Parameters
+    public static List<Object> parameters() {
+      return Arrays.asList(1, 2, 3);
+    }
+  }
+
+  @Test
+  public void testNumParameters() {
+    // Invalid cases, expected to return 1.
+    assertEquals(HBaseClassTestRule.getNumParameters(NonParameterizedClass.class), 1);
+    assertEquals(HBaseClassTestRule.getNumParameters(
+        ParameterizedClassWithNoParametersMethod.class), 1);
+    assertEquals(HBaseClassTestRule.getNumParameters(InValidParameterizedClass.class), 1);
+    // Valid parameterized classes.
+    assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass1.class),
+        ValidParameterizedClass1.parameters().size());
+    assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass2.class),
+        ValidParameterizedClass2.parameters().length);
+    assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass3.class),
+        Iterables.size(ValidParameterizedClass3.parameters()));
+    assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass4.class),
+        ValidParameterizedClass4.parameters().size());
+    // Testing inheritance.
+    assertEquals(HBaseClassTestRule.getNumParameters(ExtendedParameterizedClass1.class),
+        ValidParameterizedClass1.parameters().size());
+    assertEquals(HBaseClassTestRule.getNumParameters(ExtendedParameterizedClass2.class),
+        ExtendedParameterizedClass2.parameters().size());
+  }
+}

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

@@ -20,7 +20,6 @@
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.HConstants.HBASE_MASTER_LOGCLEANER_PLUGINS;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK;
-
 import com.google.protobuf.Descriptors;
 import com.google.protobuf.Service;
 import java.io.IOException;
@@ -56,8 +55,10 @@
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataOutputStream;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.ChoreService;
 import org.apache.hadoop.hbase.ClusterId;
@@ -219,11 +220,9 @@
 import org.eclipse.jetty.webapp.WebAppContext;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
 import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
 import org.apache.hbase.thirdparty.com.google.common.collect.Maps;
-
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.shaded.protobuf.RequestConverter;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse.CompactionState;
@@ -913,8 +912,15 @@ private void finishActiveMasterInitialization(MonitoredTask status) throws IOExc
     // hbck1s against an hbase2 cluster; it could do damage. To skip this behavior, set
     // hbase.write.hbck1.lock.file to false.
     if (this.conf.getBoolean("hbase.write.hbck1.lock.file", true)) {
-      HBaseFsck.checkAndMarkRunningHbck(this.conf,
-          HBaseFsck.createLockRetryCounterFactory(this.conf).create());
+      Pair<Path, FSDataOutputStream> result = null;
+      try {
+        result = HBaseFsck.checkAndMarkRunningHbck(this.conf,
+            HBaseFsck.createLockRetryCounterFactory(this.conf).create());
+      } finally {
+        if (result != null) {
+          IOUtils.closeQuietly(result.getSecond());
+        }
+      }
     }
 
     status.setStatus("Initialize ServerManager and schedule SCP for crash servers");