[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
Github user shaneknapp commented on a diff in the pull request:
https://github.com/apache/spark/pull/22854#discussion_r228607477
--- Diff: build/mvn ---
@@ -163,8 +163,14 @@ export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
echo "Using \`mvn\` from path: $MVN_BIN" 1>&2
-# Last, call the `mvn` command as usual
+# call the `mvn` command as usual
"${MVN_BIN}" -DzincPort=${ZINC_PORT} "$@"
+MVN_RETCODE=$?
-# Try to shut down zinc explicitly
-"${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+# SPARK-25854
+# Try to shut down zinc explicitly if the server is still running. if
it's not running,
+# it's timed out and we'll still need to exit the script w/a 0 to keep the
build from
+# failing.
+"${ZINC_BIN}" -shutdown -port ${ZINC_PORT} || true
--- End diff --
we probably don't need it, but i am more than comfortable keeping it in
there #justincase
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22854#discussion_r228606581
--- Diff: build/mvn ---
@@ -163,8 +163,14 @@ export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
echo "Using \`mvn\` from path: $MVN_BIN" 1>&2
-# Last, call the `mvn` command as usual
+# call the `mvn` command as usual
"${MVN_BIN}" -DzincPort=${ZINC_PORT} "$@"
+MVN_RETCODE=$?
-# Try to shut down zinc explicitly
-"${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+# SPARK-25854
+# Try to shut down zinc explicitly if the server is still running. if
it's not running,
+# it's timed out and we'll still need to exit the script w/a 0 to keep the
build from
+# failing.
+"${ZINC_BIN}" -shutdown -port ${ZINC_PORT} || true
--- End diff --
do we still need `|| true`? we always return `$MVN_RETCODE` now.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
Github user shaneknapp commented on a diff in the pull request:
https://github.com/apache/spark/pull/22854#discussion_r228591941
--- Diff: build/mvn ---
@@ -163,8 +163,19 @@ export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
echo "Using \`mvn\` from path: $MVN_BIN" 1>&2
-# Last, call the `mvn` command as usual
+# call the `mvn` command as usual
"${MVN_BIN}" -DzincPort=${ZINC_PORT} "$@"
-# Try to shut down zinc explicitly
-"${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+# check to see if zinc server is still running post-build
+"${ZINC_BIN}" -status -port ${ZINC_PORT} &> /dev/null
+ZINC_STATUS=$?
+
+# Try to shut down zinc explicitly if the server is still running
--- End diff --
now that i'm a couple cups of coffee in to my morning, i'm actually back to
thinking that this might be the most elegant way of dealing w/this.
i'll update the PR to do just this, and include a comment describing why
we're doing it.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22854#discussion_r228588061
--- Diff: build/mvn ---
@@ -163,8 +163,19 @@ export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
echo "Using \`mvn\` from path: $MVN_BIN" 1>&2
-# Last, call the `mvn` command as usual
+# call the `mvn` command as usual
"${MVN_BIN}" -DzincPort=${ZINC_PORT} "$@"
-# Try to shut down zinc explicitly
-"${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+# check to see if zinc server is still running post-build
+"${ZINC_BIN}" -status -port ${ZINC_PORT} &> /dev/null
+ZINC_STATUS=$?
+
+# Try to shut down zinc explicitly if the server is still running
--- End diff --
I know it's very unlikely, but there is a chance that the zinc is timed out
between we check its status and shut it down. Since zinc will be timed out
eventually, we don't care too much about if we can shut it down successfully
here.
So how about `"${ZINC_BIN}" -shutdown -port ${ZINC_PORT} || true`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
Github user shaneknapp commented on a diff in the pull request:
https://github.com/apache/spark/pull/22854#discussion_r228584294
--- Diff: build/mvn ---
@@ -163,8 +163,19 @@ export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
echo "Using \`mvn\` from path: $MVN_BIN" 1>&2
-# Last, call the `mvn` command as usual
+# call the `mvn` command as usual
"${MVN_BIN}" -DzincPort=${ZINC_PORT} "$@"
-# Try to shut down zinc explicitly
-"${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+# check to see if zinc server is still running post-build
+"${ZINC_BIN}" -status -port ${ZINC_PORT} &> /dev/null
+ZINC_STATUS=$?
+
+# Try to shut down zinc explicitly if the server is still running
+if [ $ZINC_STATUS -eq 0 ]; then
+ # zinc is still running!
+ "${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+ exit 0
--- End diff --
i put the else in there for clarity, and in case we want to ever do
something (like report that zinc timed out etc) if the exit code on the status
is 1.
¯\_(ã)_/¯
i'm also not a fan of putting `exit 0` at the end of any bash script,
anywhere, ever.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22854#discussion_r228583684
--- Diff: build/mvn ---
@@ -163,8 +163,19 @@ export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
echo "Using \`mvn\` from path: $MVN_BIN" 1>&2
-# Last, call the `mvn` command as usual
+# call the `mvn` command as usual
"${MVN_BIN}" -DzincPort=${ZINC_PORT} "$@"
-# Try to shut down zinc explicitly
-"${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+# check to see if zinc server is still running post-build
+"${ZINC_BIN}" -status -port ${ZINC_PORT} &> /dev/null
+ZINC_STATUS=$?
+
+# Try to shut down zinc explicitly if the server is still running
+if [ $ZINC_STATUS -eq 0 ]; then
+ # zinc is still running!
+ "${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+ exit 0
--- End diff --
I suppose you just want to exit 0 outside the if-else, for clarity, but
whatever. This is all fine and can be back-ported back to 2.2
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22854: [SPARK-25854] fix mvn to not always exit 1
GitHub user shaneknapp opened a pull request: https://github.com/apache/spark/pull/22854 [SPARK-25854] fix mvn to not always exit 1 ## What changes were proposed in this pull request? the final line in the mvn helper script in build/ attempts to shut down the zinc server. due to the zinc server being set up w/a 30min timeout, by the time the mvn test instantiation finishes, the server times out. this means that when the mvn script tries to shut down zinc, it returns w/an exit code of 1. this will then automatically fail the entire build (even if the build passes). ## How was this patch tested? i set up a test build: https://amplab.cs.berkeley.edu/jenkins/job/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/ You can merge this pull request into a Git repository by running: $ git pull https://github.com/shaneknapp/spark fix-mvn-helper-script Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/22854.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #22854 commit 2f0a91b64ee312bb5fb33885118afac4b7d0dc91 Author: shane knapp Date: 2018-10-26T16:01:30Z fix mvn to not always exit 1 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
