-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPARK-9545, SPARK-9547: Use Maven in PRB if title contains "[test-maven]" #7878
Conversation
This is just some small glue code to actually make use of the AMPLAB_JENKINS_BUILD_TOOL switch. As far as I can tell, we actually don't currently use the Maven support in the tool. This patch switches to Maven when the PR title contains "maven-test". There are a few small other pieces of cleanup in the patch as well.
/cc @JoshRosen @srowen and @rxin for any thoughts on this. |
Can it be in the pull request description or the comments? |
@rxin unfortunately neither of those is supported by the current PRB infrastructure. We only have access to a limited number of contextual variables set by the PRB plugin, and neither the description or comments are available. |
@rxin I've also just added support for triggering different hadoop version builds. |
Test build #39457 has finished for PR 7878 at commit
|
Test build #39454 has finished for PR 7878 at commit
|
Test build #39460 has finished for PR 7878 at commit
|
Test build #39464 has finished for PR 7878 at commit
|
@@ -231,7 +231,7 @@ def exec_maven(mvn_args=()): | |||
"""Will call Maven in the current directory with the list of mvn_args passed | |||
in and returns the subprocess for any further processing""" | |||
|
|||
run_cmd([os.path.join(SPARK_HOME, "build", "mvn")] + mvn_args) | |||
run_cmd([os.path.join(SPARK_HOME, "build", "mvn")] + ["--force"] + mvn_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the --force
can be in the first list, but it doesn't really matter.
Just so that you're aware: the diff-dependent test selection logic is not currently implemented for Maven, so pull requests which specify Maven testing will cause all tests to be run. |
Jenkins, retest this please. |
|
||
cmd = ("/usr/sbin/lsof -P |grep %s | grep LISTEN " | ||
"| awk '{ print $2; }' | xargs kill") % zinc_port | ||
# TODO: Not sure what happens here if no process exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a non-zero exit code, run_cmd
will print an error message and will call sys.exit()
, so we may want to ensure that this always returns a zero exit code. If this is a concern, you could always just perform the underlying subprocess
call yourself.
Jenkins, retest this please. |
Test build #39471 has finished for PR 7878 at commit
|
Jenkins, retest this please. |
Test build #39468 has finished for PR 7878 at commit
|
Jenkins, test this please. |
Test build #39475 has finished for PR 7878 at commit
|
Test build #39476 has finished for PR 7878 at commit
|
Jenkins, retest this please. |
Test build #39478 timed out for PR 7878 at commit |
Test build #39511 has finished for PR 7878 at commit
|
Jenkins, retest this please. |
Test build #39551 has finished for PR 7878 at commit
|
Jenkins, retest this please. |
Test build #39590 has finished for PR 7878 at commit
|
Test build #39623 has finished for PR 7878 at commit
|
Test build #39645 has finished for PR 7878 at commit
|
Jenkins, test this please. |
Conflicts: dev/run-tests-jenkins
Jenkins, test this please. |
Test build #41711 has finished for PR 7878 at commit
|
Jenkins, test this please. |
Test build #41726 has finished for PR 7878 at commit
|
Jenkins, test this please. |
Okay this seems to be passing now - any thoughts @JoshRosen or @vanzin? IMO this would be really nice since we can test build changes with either build, before they make it into spark. |
Test build #41756 has finished for PR 7878 at commit
|
I think this is a good initial step; like Reynold, I'd prefer if this was read from the PR's description. Perhaps the code can be amended later to get the description using the github API and parsing it (and then you can get fancy with how to declare test parameters too). Also, it seems like the script to merge prs won't clean up these test directives, right? That's more stuff to remember when merging PRs... On the nits side:
But overall I'm fine with it, we can keep on improving this in later PRs. |
""" | ||
Get a randomized port on which to start Zinc | ||
""" | ||
return random.randrange(3030, 4030) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this doesn't lead to builds failing because zinc can't bind to the chosen port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is identical to that hard coded in the bash scripts that run the maven builds, in the past they've never failed for this reason... but yeah it could happen.
Thanks for looking at this @vanzin. I do agree it would be a lot nicer to base things on comments, but because the comment stream isn't available as meta-data on jenkins, that's a huge amount of additional work that IMO is best left to an extension if someone is feeling interested. Given that it seems like you are cool to merge this for now as is, then look at evolving it more later. |
This is just some small glue code to actually make use of the
AMPLAB_JENKINS_BUILD_TOOL switch. As far as I can tell, we actually
don't currently use the Maven support in the tool even though it exists.
This patch switches to Maven when the PR title contains "test-maven".
There are a few small other pieces of cleanup in the patch as well.