Java test fixes: reduce default number of tablet per TS, log spew, ru…

…nning time for some tests

Summary:
Java test fixes:
- Reduce the default number of tablet per tablet server from 3 to 2.
- Change log level from DEBUG to INFO for a couple of frequent log messages from our custom load balancing policy for Cassandra driver.
- Speed up BaseCQLTest.setupTable by using multiple threads.
- In TestYBClient.testLeaderStepDown add a retry loop waiting for master leader to be elected instead of a fixed delay.
- Replace some comments with log messages so it is clearer from the log where we are in the test.

yb_build.sh / common-build-env.sh / common-test-env.sh :
- Allow specifying parallelism for repeated unit test runs without the pass-through "--" syntax,
- Refactor Maven invocations and extract common options.
- Fix a bug with yb_build.sh always trying to ssh into a remote server even if building locally and not running any tests.
- Fix a bug with not building Java code before running a single Java test from the command line.

Test Plan: Jenkins

Reviewers: oleg, pritam.damania, hector

Reviewed By: hector

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D4442

bin/repeat_unit_test.sh

@@ -76,7 +76,7 @@ fi
 positional_args=()
 more_test_args=""
 log_dir=""
-declare -i parallelism=4
+declare -i parallelism=$DEFAULT_REPEATED_TEST_PARALLELISM
 declare -i iteration=0
 declare -i num_iter=1000
 keep_all_logs=false
@@ -106,6 +106,8 @@ while [[ $# -gt 0 ]]; do
     ;;
     -p|--parallelism)
       parallelism=$2
+      validate_numeric_arg_range "parallelism" "$parallelism" \
+        "$MIN_REPEATED_TEST_PARALLELISM" "$MAX_REPEATED_TEST_PARALLELISM"
       shift
     ;;
     --log-dir)

build-support/common-build-env.sh

@@ -712,6 +712,13 @@ set_mvn_parameters() {
     fi
   fi
   export MVN_SETTINGS_PATH
+
+  mvn_common_options=(
+    --batch-mode
+    -Dmaven.repo.local="$YB_MVN_LOCAL_REPO"
+    -Dyb.thirdparty.dir="$YB_THIRDPARTY_DIR"
+    -DbinDir="$BUILD_ROOT/bin"
+  )
 }
 
 # A utility function called by both 'build_yb_java_code' and 'build_yb_java_code_with_retries'.
@@ -728,11 +735,7 @@ build_yb_java_code_filter_save_output() {
     local java_build_output_path=/tmp/yb-java-build-$( get_timestamp ).$$.tmp
     has_local_output=true
   fi
-  local mvn_opts=(
-    --batch-mode
-    -Dyb.thirdparty.dir="$YB_THIRDPARTY_DIR"
-    -Dmaven.repo.local="$YB_MVN_LOCAL_REPO"
-  )
+  local mvn_opts=( "${mvn_common_options[@]}" )
   if [[ -f $YB_MVN_SETTINGS_PATH  ]]; then
     mvn_opts+=(
       --settings "$YB_MVN_SETTINGS_PATH"
@@ -1564,6 +1567,21 @@ handle_build_root_from_current_dir() {
   fatal "Working directory of the compiler '$PWD' is not within a valid YugaByte build root."
 }
 
+validate_numeric_arg_range() {
+  expect_num_args 4 "$@"
+  local arg_name=$1
+  local arg_value=$2
+  local -r -i min_value=$3
+  local -r -i max_value=$4
+  if [[ ! $arg_value =~ ^[0-9]+$ ]]; then
+    fatal "Invalid numeric argument value for --$arg_name: '$arg_value'"
+  fi
+  if [[ $arg_value -lt $min_value || $arg_value -gt $max_value ]]; then
+    fatal "Value out of range for --$arg_name: $arg_value, must be between $min_value and" \
+          "$max_value."
+  fi
+}
+
 # -------------------------------------------------------------------------------------------------
 # Initialization
 # -------------------------------------------------------------------------------------------------

build-support/common-test-env.sh

@@ -80,6 +80,10 @@ readonly JENKINS_NFS_BUILD_REPORT_BASE_DIR="/n/jenkins/build_stats"
 # https://github.com/google/sanitizers/wiki/SanitizerCommonFlags
 readonly SANITIZER_COMMON_OPTIONS=""
 
+declare -i -r MIN_REPEATED_TEST_PARALLELISM=1
+declare -i -r MAX_REPEATED_TEST_PARALLELISM=100
+declare -i -r DEFAULT_REPEATED_TEST_PARALLELISM=4
+
 # -------------------------------------------------------------------------------------------------
 # Functions
 # -------------------------------------------------------------------------------------------------
@@ -1275,6 +1279,50 @@ fix_cxx_test_name() {
   fi
 }
 
+run_java_test() {
+  expect_num_args 2 "$@"
+  local module_name=$1
+  local test_class=$2
+  if [[ -z ${BUILD_ROOT:-} ]]; then
+    fatal "Running Java tests requires that BUILD_ROOT be set"
+  fi
+  set_mvn_parameters
+  set_sanitizer_runtime_options
+  mkdir -p "$YB_TEST_LOG_ROOT_DIR/java"
+
+  # This can't include $YB_TEST_INVOCATION_ID -- previously, when we did that, it looked like some
+  # Maven processes were killed, although it is not clear why, because they should have already
+  # completed by the time we start looking for $YB_TEST_INVOCATION_ID in test names and killing
+  # processes.
+  local surefire_rel_tmp_dir=surefire${timestamp}_${RANDOM}_${RANDOM}_${RANDOM}_$$
+
+  cd "$YB_SRC_ROOT/java"
+  # We specify tempDir to use a separate temporary directory for each test.
+  # http://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html
+  mvn_options=(
+    "${mvn_common_options[@]}"
+
+    -Dtest="$test_class"
+    --projects "$module_name"
+    -DtempDir="$surefire_rel_tmp_dir"
+    -DskipAssembly
+    -Dmaven.javadoc.skip
+  )
+
+  if is_jenkins || \
+     [[ $YB_MVN_SETTINGS_PATH != "$HOME/.m2/settings.xml" ]] || \
+     [[ -f $YB_MVN_SETTINGS_PATH ]]; then
+    mvn_options+=( --settings "$YB_MVN_SETTINGS_PATH" )
+  fi
+
+  if is_jenkins; then
+    # When running on Jenkins we'd like to see more debug output.
+    mvn_options+=( -X )
+  fi
+
+  ( set -x; mvn "${mvn_options[@]}" surefire:test )
+}
+
 # -------------------------------------------------------------------------------------------------
 # Initialization
 # -------------------------------------------------------------------------------------------------

build-support/run-test.sh

@@ -74,51 +74,14 @@ timestamp=$( get_timestamp_for_filenames )
 export YB_TEST_INVOCATION_ID=${timestamp}_${RANDOM}_${RANDOM}_$$
 trap cleanup EXIT
 
+set_common_test_paths
+
 if [[ $# -eq 2 && -d $YB_SRC_ROOT/java/$1 ]]; then
   # This is a Java test.
   # Arguments: <maven_module_name> <package_and_class>
   # Example: yb-client org.yb.client.TestYBClient
-  module_name=$1
-  test_class=$2
-  if [[ -z ${BUILD_ROOT:-} ]]; then
-    fatal "Running Java tests with run-test.sh requires that BUILD_ROOT be set"
-  fi
-  set_common_test_paths
-  set_mvn_parameters
-  set_sanitizer_runtime_options
-  mkdir -p "$YB_TEST_LOG_ROOT_DIR/java"
-
-  # This can't include $YB_TEST_INVOCATION_ID -- previously, when we did that, it looked like some
-  # Maven processes were killed, although it is not clear why, because they should have already
-  # completed by the time we start looking for $YB_TEST_INVOCATION_ID in test names and killing
-  # processes.
-  surefire_rel_tmp_dir=surefire${timestamp}_${RANDOM}_${RANDOM}_${RANDOM}_$$
-
-  cd "$YB_SRC_ROOT/java"
-  # We specify tempDir to use a separate temporary directory for each test.
-  # http://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html
-  mvn_options=(
-    -Dtest="$test_class"
-    --projects "$module_name"
-    -DbinDir="$BUILD_ROOT/bin"
-    -Dmaven.repo.local="$YB_MVN_LOCAL_REPO"
-    -DtempDir="$surefire_rel_tmp_dir"
-    -DskipAssembly
-    -Dmaven.javadoc.skip
-  )
-
-  if is_jenkins || \
-     [[ $YB_MVN_SETTINGS_PATH != "$HOME/.m2/settings.xml" ]] || \
-     [[ -f $YB_MVN_SETTINGS_PATH ]]; then
-    mvn_options+=( --settings "$YB_MVN_SETTINGS_PATH" )
-  fi
-
-  if is_jenkins; then
-    # When running on Jenkins we'd like to see more debug output.
-    mvn_options+=( -X )
-  fi
 
-  ( set -x; mvn "${mvn_options[@]}" surefire:test )
+  run_java_test "$@"
   # See the cleanup() function above for how we kill stuck processes based on the
   # $YB_TEST_INVOCATION_ID pattern.
   exit

java/yb-client/src/test/java/org/yb/BaseYBTest.java

@@ -63,7 +63,6 @@ public class BaseYBTest {
   public final TestRule watcher = new TestWatcher() {
 
     boolean succeeded;
-    boolean failed;
     long startTimeMs;
     Throwable failureException;
 
@@ -107,7 +106,6 @@ private void switchLogging(Runnable doLogging, Runnable doSwitching) {
 
     private void resetState() {
       succeeded = false;
-      failed = false;
       startTimeMs = 0;
       failureException = null;
     }
@@ -152,6 +150,7 @@ protected void starting(Description description) {
     @Override
     protected void succeeded(Description description) {
       super.succeeded(description);
+      succeeded = true;
     }
 
     @Override
@@ -173,7 +172,7 @@ protected void finished(Description description) {
             if (succeeded) {
               LOG.info("YB Java test succeeded: " + descStr);
             }
-            if (failed) {
+            if (failureException != null) {
               LOG.error("YB Java test failed: " + descStr, failureException);
             }
             if (startTimeMs == 0) {

java/yb-client/src/test/java/org/yb/client/TestUtils.java

@@ -616,8 +616,7 @@ public static String getTestReportFilePrefix() {
     }
     return new File(
         surefireDir,
-        testClass.getName() + String.format(".%03d.", BaseYBTest.getTestMethodIndex()) +
-            BaseYBTest.getCurrentTestMethodName() + "."
+        testClass.getName() + "."  + BaseYBTest.getCurrentTestMethodName() + "."
     ).toString();
   }
 

java/yb-client/src/test/java/org/yb/client/TestYBClient.java

@@ -37,11 +37,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 import org.junit.After;
 import org.junit.Before;
@@ -202,8 +198,13 @@ public void testLeaderStepDown() throws Exception {
     int numBefore = listResp.getMasters().size();
     LeaderStepDownResponse resp = syncClient.masterLeaderStepDown();
     assertFalse(resp.hasError());
-    // Sleep to give time for re-election completion. TODO: Add api for election completion.
+    // We might have failed due to election taking too long in this instance: https://goo.gl/xYmZDb.
     Thread.sleep(6000);
+    TestUtils.waitFor(
+        () -> {
+          return syncClient.getLeaderMasterUUID() != null;
+        }, 20000);
+
     String newLeaderUuid = syncClient.getLeaderMasterUUID();
     assertNotNull(newLeaderUuid);
     listResp = syncClient.listMasters();

java/yb-client/src/test/java/org/yb/minicluster/BaseMiniClusterTest.java

@@ -45,10 +45,14 @@ public class BaseMiniClusterTest extends BaseYBTest {
   protected static final int NUM_MASTERS =
       Integer.getInteger(NUM_MASTERS_PROP, DEFAULT_NUM_MASTERS);
 
-  /** This is used as the default timeout when calling YB Java client's async API. */
+  /**
+   * This is used as the default timeout when calling YB Java client's async API.
+   */
   protected static final int DEFAULT_SLEEP = 50000;
 
-  /** A mini-cluster shared between invocations of multiple test methods. */
+  /**
+   * A mini-cluster shared between invocations of multiple test methods.
+   */
   protected static MiniYBCluster miniCluster;
 
   protected static List<String> masterArgs = null;
@@ -58,6 +62,11 @@ public class BaseMiniClusterTest extends BaseYBTest {
   protected static String masterAddresses;
   protected static List<HostAndPort> masterHostPorts;
 
+  // Subclasses can override this to set the number of shards per tablet server.
+  protected int overridableNumShardsPerTServer() {
+    return MiniYBCluster.DEFAULT_NUM_SHARDS_PER_TSERVER;
+  }
+
   /**
    * This makes sure that the mini cluster is up and running before each test. A test might opt to
    * leave the mini cluster running, and it will be reused by next tests, or it might shut down the
@@ -107,6 +116,7 @@ public void createMiniCluster(int numMasters, List<List<String>> tserverArgs)
                       .testClassName(getClass().getName())
                       .masterArgs(masterArgs)
                       .tserverArgs(tserverArgs)
+                      .numShardsPerTServer(overridableNumShardsPerTServer())
                       .build();
     masterAddresses = miniCluster.getMasterAddresses();
     masterHostPorts = miniCluster.getMasterHostPorts();

java/yb-client/src/test/java/org/yb/minicluster/MiniYBCluster.java

@@ -125,12 +125,9 @@ public class MiniYBCluster implements AutoCloseable {
   private AtomicInteger nextMasterIndex = new AtomicInteger(0);
   private AtomicInteger nextTServerIndex = new AtomicInteger(0);
 
-  private static final int DEFAULT_NUM_SHARDS_PER_TSERVER = 3;
-  private static int numShardsPerTserver = DEFAULT_NUM_SHARDS_PER_TSERVER;
+  public static final int DEFAULT_NUM_SHARDS_PER_TSERVER = 3;
 
-  public static void setNumShardsPerTserver(int numShards) {
-    numShardsPerTserver = numShards;
-  }
+  private int numShardsPerTserver = DEFAULT_NUM_SHARDS_PER_TSERVER;
 
   /**
    * Hard memory limit for YB daemons. This should be consistent with the memory limit set for C++
@@ -147,9 +144,11 @@ public static void setNumShardsPerTserver(int numShards) {
                 int defaultTimeoutMs,
                 List<String> masterArgs,
                 List<List<String>> tserverArgs,
+                int numShardsPerTserver,
                 String testClassName) throws Exception {
     this.defaultTimeoutMs = defaultTimeoutMs;
     this.testClassName = testClassName;
+    this.numShardsPerTserver = numShardsPerTserver;
 
     startCluster(numMasters, numTservers, masterArgs, tserverArgs);
 

java/yb-client/src/test/java/org/yb/minicluster/MiniYBClusterBuilder.java

@@ -20,6 +20,7 @@ public class MiniYBClusterBuilder {
 
   private int numMasters = 1;
   private int numTservers = 3;
+  private int numShardsPerTServer = MiniYBCluster.DEFAULT_NUM_SHARDS_PER_TSERVER;
   private int defaultTimeoutMs = 50000;
   private List<String> masterArgs = null;
   private List<List<String>> tserverArgs = null;
@@ -35,6 +36,11 @@ public MiniYBClusterBuilder numTservers(int numTservers) {
     return this;
   }
 
+  public MiniYBClusterBuilder numShardsPerTServer(int numShardsPerTServer) {
+    this.numShardsPerTServer = numShardsPerTServer;
+    return this;
+  }
+
   /**
    * Configures the internal client to use the given timeout for all operations. Also uses the
    * timeout for tasks like waiting for tablet servers to check in with the master.
@@ -75,6 +81,6 @@ public MiniYBClusterBuilder testClassName(String testClassName) {
 
   public MiniYBCluster build() throws Exception {
     return new MiniYBCluster(numMasters, numTservers, defaultTimeoutMs, masterArgs, tserverArgs,
-        testClassName);
+        numShardsPerTServer, testClassName);
   }
 }

java/yb-client/src/test/resources/log4j.properties

@@ -51,3 +51,7 @@ log4j.logger.com.datastax.driver.core = INFO
 # Deferred@1539947167(state=PAUSED, result=Deferred@1611366759, callback=wakeup thread main,
 # errback=wakeup thread main)
 log4j.logger.com.stumbleupon.async.Deferred = INFO
+
+# To avoid log spew like https://goo.gl/1rhr8F in unit tests.
+log4j.logger.com.yugabyte.driver.core.policies.PartitionAwarePolicy = INFO
+log4j.logger.com.yugabyte.driver.core.TableSplitMetadata = INFO