[SPARK-19482][CORE] Fail it if 'spark.master' is set with different value #16827
Conversation
|
retest this please. |
|
Test build #72483 has finished for PR 16827 at commit
|
|
Test build #72485 has finished for PR 16827 at commit
|
|
Test build #72486 has finished for PR 16827 at commit
|
|
Working on UT failure. |
| val commandLineArgs = Array( | ||
| "--deploy-mode", "cluster", | ||
| "--master", masterUrl, | ||
| "--name", mainClass, |
There was a problem hiding this comment.
I think this is a useless setting, it will be rewritten in rest server side with right value.
| val conf = new SparkConf(false).setAppName("My app") | ||
| sc = new SparkContext("local[2]", "My other app", conf) | ||
| assert(sc.master === "local[2]") | ||
| assert(sc.appName === "My other app") |
There was a problem hiding this comment.
remove it, as it can not be overrode.
| assert(Utils.isDynamicAllocationEnabled( | ||
| conf.set("spark.executor.instances", "0")) === true) | ||
| conf.remove("spark.master") | ||
| assert(Utils.isDynamicAllocationEnabled(conf.set("spark.master", "local")) === false) |
There was a problem hiding this comment.
trick for unit test, otherwise fail.
| newReloadConf.getOption(prop).foreach { value => | ||
| // (trick) avoid SPARK-19482. | ||
| newSparkConf.remove(prop) | ||
| newSparkConf.set(prop, value) |
There was a problem hiding this comment.
here, we should remove properties which will be reloaded, otherwise will fail for some special properties, like 'spark.master'
| } | ||
| if (previousOne.isDefined && !previousOne.get.equals(value)) { | ||
| throw new IllegalArgumentException(s"'spark.master' should not be set with different " + | ||
| s"values, previous value is ${previousOne.get} and current value is $value.") |
There was a problem hiding this comment.
here, we choose to throw IllegalArgumentException to fail job. Besides, we can also choose to log a warning in a gentle way.
|
I don't think we want or need to special-case behavior of this one property. It's also a behavior change. |
|
Test build #72494 has finished for PR 16827 at commit
|
|
@srowen Well, this change may be really radical. Just like what I say, many fresh users has no consciousness to check if job runs correctly in standalone mode or yarn mode. Maybe, they just forgot to remove the "local[*]" from code after debug or just did not know the priority of setting 'spark.master'. This is not a rare case. I have have received many reports like this. Indeed, we can say only newbie may make this mistake. But it is better to warn users in some ways. What about just log a warning? But, I don't think users will notice it. Besides, I do not find any strong reason to set 'spark.master' multi-times with different value. Any suggestion is appreciated. |
|
I still don't see a reason to special-case this property. It also means you can't override a property set by defaults, which could be a problem. |
|
@srowen make senses. What about logging a warning/error message if 'spark.master' is set with different values? |
|
I just don't think that setting properties to override defaults, which is the common reason this would occur, is a warning or error situation. |
|
@srowen make sense, close it first before there is a follow-up |
3 participants
What changes were proposed in this pull request?
First, there is no need to set 'spark.master' multi-times with different values. Second, It is possible for users to set the different 'spark.master' in code with
spark-submitcommand, and will confuse himself. So, we should do once check if the 'spark.master' already exists in settings and if the previous value is the same with current value. Throw a IllegalArgumentException when previous value is different with current value.BTW, I have received many cases that users forgot to remove "local[*]" from code and submit it in command line. And many fresh users has no consciousness to check if job runs correctly in standalone mode or yarn mode. At last, the job will run on master node or gateway node all along.
How was this patch tested?
add new unit test