[SPARK-25854][BUILD] fix `build/mvn` not to fail during Zinc server shutdown #22854

shaneknapp · 2018-10-26T16:03:21Z

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/

srowen · 2018-10-26T16:08:43Z

build/mvn

+if [ $ZINC_STATUS -eq 0 ]; then
+  # zinc is still running!
+  "${ZINC_BIN}" -shutdown -port ${ZINC_PORT}
+  exit 0


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

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.

SparkQA · 2018-10-26T16:09:05Z

Test build #98094 has started for PR 22854 at commit 2f0a91b.

cloud-fan · 2018-10-26T16:23:22Z

build/mvn

+"${ZINC_BIN}" -status -port ${ZINC_PORT} &> /dev/null
+ZINC_STATUS=$?
+
+# Try to shut down zinc explicitly if the server is still running


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?

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.

shaneknapp · 2018-10-26T16:41:04Z

test this please

SparkQA · 2018-10-26T16:43:54Z

Test build #98099 has started for PR 22854 at commit 627e7d8.

shaneknapp · 2018-10-26T16:54:50Z

here's the test build run that's actually testing the changes to mvn:
https://amplab.cs.berkeley.edu/jenkins/job/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/12/

shaneknapp · 2018-10-26T17:03:50Z

mvn clean package script logic worked!

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 15:11 min
[INFO] Finished at: 2018-10-26T10:01:13-07:00
[INFO] ------------------------------------------------------------------------
+ MVN_RETCODE=0
+ /home/jenkins/workspace/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/build/zinc-0.3.15/bin/zinc -shutdown -port 3087
+ exit 0     <--------------  this is from "exit $MVN_RETCODE"
+ retcode1=0

vanzin · 2018-10-26T17:04:48Z

Probably unnecessary to change the timeout, but LGTM.

cloud-fan · 2018-10-26T17:28:46Z

build/mvn

+# 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


do we still need || true? we always return $MVN_RETCODE now.

we probably don't need it, but i am more than comfortable keeping it in there #justincase

alright, i tested the script w/a false call in the mvn helper script and nothing broke. pushed a change removing || true.

dongjoon-hyun

+1, LGTM.

Thank you, @shaneknapp . I also wondered the reason of these failures. BTW, could you update the title a little bit? For example,

[SPARK-25854] fix mvn to not always exit 1
[SPARK-25854][BUILD] Fix `build/mvn` not to fail during Zinc server shutdown

shaneknapp · 2018-10-26T17:42:43Z

let's not merge until this build passes:
https://amplab.cs.berkeley.edu/jenkins/job/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/12/

(i hacked this build to scp the updated mvn helper script over before compilation starts)

shaneknapp · 2018-10-26T18:08:51Z

Merged build finished. Test FAILed.

that was me killing the 1st PRB build

shaneknapp · 2018-10-26T21:12:37Z

build is green, and everything looks to be behaving normally!

https://amplab.cs.berkeley.edu/jenkins/job/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/12/console

ready to merge and backport (@srowen could you help out w/this one?)

SparkQA · 2018-10-26T21:34:34Z

Test build #98104 has finished for PR 22854 at commit 3d32b61.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

I can merge and backport. The last test failure is spurious.

shaneknapp · 2018-10-26T21:38:14Z

yeah... wasn't worried about the PRB failing. thanks for the merge/backport!

…hutdown ## 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/ Closes #22854 from shaneknapp/fix-mvn-helper-script. Authored-by: shane knapp <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 6aa5063) Signed-off-by: Sean Owen <[email protected]>

…hutdown 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). i set up a test build: https://amplab.cs.berkeley.edu/jenkins/job/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/ Closes #22854 from shaneknapp/fix-mvn-helper-script. Authored-by: shane knapp <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 6aa5063) Signed-off-by: Sean Owen <[email protected]>

SparkQA · 2018-10-26T22:15:24Z

Test build #98103 has finished for PR 22854 at commit 45bb7b0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…hutdown ## 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/ Closes apache#22854 from shaneknapp/fix-mvn-helper-script. Authored-by: shane knapp <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…hutdown ## 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/ Closes apache#22854 from shaneknapp/fix-mvn-helper-script. Authored-by: shane knapp <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 6aa5063) Signed-off-by: Sean Owen <[email protected]>

…hutdown 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). i set up a test build: https://amplab.cs.berkeley.edu/jenkins/job/sknapp-testing-spark-branch-2.4-test-maven-hadoop-2.7/ Closes apache#22854 from shaneknapp/fix-mvn-helper-script. Authored-by: shane knapp <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 6aa5063) Signed-off-by: Sean Owen <[email protected]>

fix mvn to not always exit 1

2f0a91b

srowen reviewed Oct 26, 2018

View reviewed changes

cloud-fan reviewed Oct 26, 2018

View reviewed changes

shaneknapp added 2 commits October 26, 2018 09:38

simplify and add lightness

d38efe4

add jira issue to the comment for future archaeology

d09f4c0

we should exit this script with the mvn exit code

627e7d8

cloud-fan reviewed Oct 26, 2018

View reviewed changes

dongjoon-hyun approved these changes Oct 26, 2018

View reviewed changes

shaneknapp changed the title ~~[SPARK-25854] fix mvn to not always exit 1~~ [SPARK-25854][BUILD] fix build/mvn not to fail during Zinc server shutdown Oct 26, 2018

shaneknapp added 2 commits October 26, 2018 11:01

removing superfluous true

45bb7b0

updating comments

3d32b61

srowen approved these changes Oct 26, 2018

View reviewed changes

asfgit closed this in 6aa5063 Oct 26, 2018

shaneknapp deleted the fix-mvn-helper-script branch April 19, 2019 17:35

[SPARK-25854][BUILD] fix build/mvn not to fail during Zinc server shutdown #22854

[SPARK-25854][BUILD] fix build/mvn not to fail during Zinc server shutdown #22854

Uh oh!

Conversation

shaneknapp commented Oct 26, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

shaneknapp Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 26, 2018

Uh oh!

cloud-fan Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

shaneknapp Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

shaneknapp commented Oct 26, 2018

Uh oh!

SparkQA commented Oct 26, 2018

Uh oh!

shaneknapp commented Oct 26, 2018

Uh oh!

shaneknapp commented Oct 26, 2018

Uh oh!

vanzin commented Oct 26, 2018

Uh oh!

cloud-fan Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

shaneknapp Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

shaneknapp Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

shaneknapp commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaneknapp commented Oct 26, 2018

Uh oh!

shaneknapp commented Oct 26, 2018

Uh oh!

SparkQA commented Oct 26, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

shaneknapp commented Oct 26, 2018

Uh oh!

SparkQA commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-25854][BUILD] fix `build/mvn` not to fail during Zinc server shutdown #22854

[SPARK-25854][BUILD] fix `build/mvn` not to fail during Zinc server shutdown #22854

shaneknapp Oct 26, 2018 •

edited

Loading

shaneknapp commented Oct 26, 2018 •

edited

Loading