[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6650: complete compatibility regression testing

[GitBox <gi...@apache.org>]

mcvsubbu commented on a change in pull request #6650:
URL: https://github.com/apache/incubator-pinot/pull/6650#discussion_r588800523



##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -34,61 +34,45 @@
 #  and run all the scripts in the directory in alpha order, one script at each
 #  "stage" of upgrade.
 #
-#  We may modify to choose a minimal run in which the same set of operatons are run
+#  We may modify to choose a minimal run in which the same set of operations are run
 #  between any two component upgrades/rollbacks -- this may consist of adding
 #  one more segment to table, adding some more rows to the stream topic, and
 #  running some queries with the new data.
 
-# get a temporary directory in case the workingDir is not provided by user
-TMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'mytmpdir')
-
-COMPAT_TESTER_PATH="pinot-integration-tests/target/pinot-integration-tests-pkg/bin/pinot-compat-test-runner.sh"
 
 # get usage of the script
 function usage() {
   command=$1
-  echo "Usage: $command olderCommit newerCommit [workingDir]"
+  echo "Usage: $command [workingDir]"
   exit 1
 }
 
-# cleanup the temporary directory when exiting the script
-function cleanup() {
-  if [ -n "$TMP_DIR" ] && [ -d "$TMP_DIR" ] && [ "$workingDir" = "$TMP_DIR" ] ; then
-    echo "The temporary directory $TMP_DIR needs to be cleaned up."
-  fi
-}
-
-# This function builds Pinot given a specific commit hash and target directory
-function checkoutAndBuild() {
-  commitHash=$1
-  targetDir=$2
-
-  pushd "$targetDir" || exit 1
-  git init
-  git remote add origin https://github.com/apache/incubator-pinot
-  git fetch --depth 1 origin "$commitHash"
-  git checkout FETCH_HEAD
-  mvn install package -DskipTests -Pbin-dist
-  popd || exit 1
-}
-
 # Given a component and directory, start that version of the specific component
 function startService() {
   serviceName=$1
   dirName=$2
   # Upon start, save the pid of the process for a component into a file in /tmp/{component}.pid, which is then used to stop it
   pushd "$dirName"/pinot-tools/target/pinot-tools-pkg/bin  || exit 1
   if [ "$serviceName" = "zookeeper" ]; then
-    sh -c 'echo $$ > $0/zookeeper.pid; exec ./pinot-admin.sh StartZookeeper' "${dirName}" &
+    sh -c 'rm -rf /tmp/zkdir'

Review comment:
       Instead of `/tmp` use the working dir here

##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -34,61 +34,45 @@
 #  and run all the scripts in the directory in alpha order, one script at each
 #  "stage" of upgrade.
 #
-#  We may modify to choose a minimal run in which the same set of operatons are run
+#  We may modify to choose a minimal run in which the same set of operations are run
 #  between any two component upgrades/rollbacks -- this may consist of adding
 #  one more segment to table, adding some more rows to the stream topic, and
 #  running some queries with the new data.
 
-# get a temporary directory in case the workingDir is not provided by user
-TMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'mytmpdir')
-
-COMPAT_TESTER_PATH="pinot-integration-tests/target/pinot-integration-tests-pkg/bin/pinot-compat-test-runner.sh"
 
 # get usage of the script
 function usage() {
   command=$1
-  echo "Usage: $command olderCommit newerCommit [workingDir]"
+  echo "Usage: $command [workingDir]"
   exit 1
 }
 
-# cleanup the temporary directory when exiting the script
-function cleanup() {
-  if [ -n "$TMP_DIR" ] && [ -d "$TMP_DIR" ] && [ "$workingDir" = "$TMP_DIR" ] ; then
-    echo "The temporary directory $TMP_DIR needs to be cleaned up."
-  fi
-}
-
-# This function builds Pinot given a specific commit hash and target directory
-function checkoutAndBuild() {
-  commitHash=$1
-  targetDir=$2
-
-  pushd "$targetDir" || exit 1
-  git init
-  git remote add origin https://github.com/apache/incubator-pinot
-  git fetch --depth 1 origin "$commitHash"
-  git checkout FETCH_HEAD
-  mvn install package -DskipTests -Pbin-dist
-  popd || exit 1
-}
-
 # Given a component and directory, start that version of the specific component
 function startService() {
   serviceName=$1
   dirName=$2
   # Upon start, save the pid of the process for a component into a file in /tmp/{component}.pid, which is then used to stop it
   pushd "$dirName"/pinot-tools/target/pinot-tools-pkg/bin  || exit 1
   if [ "$serviceName" = "zookeeper" ]; then
-    sh -c 'echo $$ > $0/zookeeper.pid; exec ./pinot-admin.sh StartZookeeper' "${dirName}" &
+    sh -c 'rm -rf /tmp/zkdir'
+    sh -c 'echo $$ > $0/zookeeper.pid; exec ./pinot-admin.sh StartZookeeper -dataDir /tmp/zkdir > ${0}/zookeeper.log 2>&1' "${dirName}" &
   elif [ "$serviceName" = "controller" ]; then
-    sh -c 'echo $$ > $0/controller.pid; exec ./pinot-admin.sh StartController' "${dirName}" &
+    sh -c 'echo $$ > $0/controller.pid; exec ./pinot-admin.sh StartController > ${0}/controller.log 2>&1' "${dirName}" &
   elif [ "$serviceName" = "broker" ]; then
-    sh -c 'echo $$ > $0/broker.pid; exec ./pinot-admin.sh StartBroker' "${dirName}" &
+    sh -c 'echo $$ > $0/broker.pid; exec ./pinot-admin.sh StartBroker > ${0}/broker.log 2>&1' "${dirName}" &
   elif [ "$serviceName" = "server" ]; then
-    sh -c 'echo $$ > $0/server.pid; exec ./pinot-admin.sh StartServer' "${dirName}" &
+    sh -c 'echo $$ > $0/server.pid; exec ./pinot-admin.sh StartServer > ${0}/server.log 2>&1' "${dirName}" &
   elif [ "$serviceName" = "kafka" ]; then
-    sh -c 'echo $$ > $0/kafka.pid; exec ./pinot-admin.sh StartKafka -zkAddress localhost:2181/kafka' "${dirName}" &
+    sh -c 'echo $$ > $0/kafka.pid; exec ./pinot-admin.sh StartKafka -zkAddress localhost:2181/kafka > ${0}/kafka.log 2>&1' "${dirName}" &
   fi
+
+  # sleep 60s to allow service to do some initialization:
+  # 1. controller depends on zookeeper, if not wait zookeeper to be ready, controller will crash.
+  # 2. broker depends on controller, if not wait controller to be ready, broker will crash.

Review comment:
       you should include these sleeps in the start services method. Do the following:
   - start Zookeeper
   - call waitForZkReady
   - start controller
   - call waitForControllerReady
   - start server
   - start broker
   - call waitForClusterReady
   
   For now, in each of the callForXXXReady methods, just put a sleep for 10s, and add a TODO. We will add a check for zk, controller, etc. maybe in java via yaml file or using shell commands later.

##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -217,29 +150,87 @@ if [ "$(lsof -t -i:8097 -s TCP:LISTEN)" ] || [ "$(lsof -t -i:8098 -sTCP:LISTEN)"
   exit 1
 fi
 
-# Setup initial cluster with olderCommit and do rolling upgrade
+# Setup initial cluster with olderCommit
 startServices "$oldTargetDir"
-#$COMPAT_TESTER pre-controller-upgrade.yaml; if [ $? -ne 0 ]; then exit 1; fi
+
+$COMPAT_TESTER "offline-table-create-op.yaml" 1; if [ $? -ne 0 ]; then exit 1; fi

Review comment:
       Don't add these yet. My plan is to change the compat-tester to take a dir name and execute all yamls under the directory. For now, just leaving one call commented as a placeholder is enough.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org