[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21168#discussion_r184506540
--- Diff:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
---
@@ -675,9 +675,14 @@ private[spark] class ApplicationMaster(args:
ApplicationMasterArguments) extends
val userThread = new Thread {
override def run() {
try {
- mainMethod.invoke(null, userArgs.toArray)
- finish(FinalApplicationStatus.SUCCEEDED,
ApplicationMaster.EXIT_SUCCESS)
- logDebug("Done running users class")
+ if(!Modifier.isStatic(mainMethod.getModifiers)) {
--- End diff --
This won't pass the style checker...
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21168#discussion_r184506499
--- Diff:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
---
@@ -675,9 +675,14 @@ private[spark] class ApplicationMaster(args:
ApplicationMasterArguments) extends
val userThread = new Thread {
override def run() {
try {
- mainMethod.invoke(null, userArgs.toArray)
- finish(FinalApplicationStatus.SUCCEEDED,
ApplicationMaster.EXIT_SUCCESS)
- logDebug("Done running users class")
+ if(!Modifier.isStatic(mainMethod.getModifiers)) {
+logError(s"Could not find main method in object
${args.userClass}")
--- End diff --
The message could be a little more helpful here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
Github user eric-maynard commented on a diff in the pull request:
https://github.com/apache/spark/pull/21168#discussion_r184442769
--- Diff:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
---
@@ -675,9 +675,14 @@ private[spark] class ApplicationMaster(args:
ApplicationMasterArguments) extends
val userThread = new Thread {
override def run() {
try {
- mainMethod.invoke(null, userArgs.toArray)
- finish(FinalApplicationStatus.SUCCEEDED,
ApplicationMaster.EXIT_SUCCESS)
- logDebug("Done running users class")
+ if(mainMethod == null) {
--- End diff --
Yes, I think you are definitely correct. It cannot be null. @vanzin is
right, and the check should instead ensure the `main` being invoked is static.
PR is updated, but I may decline as something is wrong with the way I am
testing.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21168#discussion_r184439831
--- Diff:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
---
@@ -675,9 +675,14 @@ private[spark] class ApplicationMaster(args:
ApplicationMasterArguments) extends
val userThread = new Thread {
override def run() {
try {
- mainMethod.invoke(null, userArgs.toArray)
- finish(FinalApplicationStatus.SUCCEEDED,
ApplicationMaster.EXIT_SUCCESS)
- logDebug("Done running users class")
+ if(mainMethod == null) {
--- End diff --
Also, I don't think this can return `null`. It should return no such method
error if not found or return the method.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
Github user eric-maynard commented on a diff in the pull request:
https://github.com/apache/spark/pull/21168#discussion_r184435342
--- Diff:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
---
@@ -675,9 +675,14 @@ private[spark] class ApplicationMaster(args:
ApplicationMasterArguments) extends
val userThread = new Thread {
override def run() {
try {
- mainMethod.invoke(null, userArgs.toArray)
- finish(FinalApplicationStatus.SUCCEEDED,
ApplicationMaster.EXIT_SUCCESS)
- logDebug("Done running users class")
+ if(mainMethod == null) {
--- End diff --
Good question -- I was also unsure of this myself.
Ultimately I *was* able to replicate the issue described in the JIRA, this
PR did solve the issue. Also, the NPE in the JIRA stracktrace does indeed point
to the invocation of `mainMethod.invoke`. So tentatively I think the answer is
'yes'
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21168#discussion_r184428171
--- Diff:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
---
@@ -675,9 +675,14 @@ private[spark] class ApplicationMaster(args:
ApplicationMasterArguments) extends
val userThread = new Thread {
override def run() {
try {
- mainMethod.invoke(null, userArgs.toArray)
- finish(FinalApplicationStatus.SUCCEEDED,
ApplicationMaster.EXIT_SUCCESS)
- logDebug("Done running users class")
+ if(mainMethod == null) {
--- End diff --
Can this be `null`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21168: added check to ensure main method is found [SPARK...
GitHub user eric-maynard opened a pull request: https://github.com/apache/spark/pull/21168 added check to ensure main method is found [SPARK-23830] ## What changes were proposed in this pull request? When a user specifies the wrong class -- or, in fact, a class instead of an object -- Spark throws an NPE which is not useful for debugging. This was reported in [SPARK-23830](https://issues.apache.org/jira/browse/SPARK-23830). This PR adds a check to ensure the main method was found and logs a useful error in the even that it's null. ## How was this patch tested? * Unit tests + Manual testing * The scope of the changes is very limited You can merge this pull request into a Git repository by running: $ git pull https://github.com/eric-maynard/spark feature/SPARK-23830 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21168.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 #21168 commit 8c68dd7ff0d17e2a5d23583dac22487b292aa00b Author: eric-maynard Date: 2018-04-26T14:58:21Z added check to ensure main method is found [SPARK-23830] --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
