-
Notifications
You must be signed in to change notification settings - Fork 117
Spark Submit changes and test #542
Conversation
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.
LGTM with one minor comment.
|
|
||
| mainClass should be ("org.apache.spark.deploy.k8s.submit.Client") | ||
| classpath should have length (0) | ||
| sysProps("spark.executor.memory") should be ("5g") |
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.
Add one for spark.driver.memory.
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.
Done. Thanks!
|
Are we missing testing logic for app resources or is that going to be a follow up PR? |
|
@ifilonenko Added the check for the jar that's submitted. Did you mean something else? The implementation details wrt resources should be covered under |
|
@foxish that was part of it, yes. But coverage in regards to this: spark/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala Lines 636 to 653 in 0add46a
|
|
Ah! I see.. I think those should be separate unit tests in a subsequent PR. |
|
rerun integration test please |
|
rerun unit tests please |
|
rerun integration test please |
1 similar comment
|
rerun integration test please |
cc @mccheah @liyinan926 @apache-spark-on-k8s/contributors