[SPARK-16435][YARN][MINOR] Add warning log if initialExecutors is less than minExecutors #14149
Conversation
|
Test build #62153 has finished for PR 14149 at commit
|
|
@tgravescs and @rdblue , please help to review, thanks a lot. |
| if (conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS) < conf.get(DYN_ALLOCATION_MIN_EXECUTORS)) { | ||
| logWarning(s"${DYN_ALLOCATION_INITIAL_EXECUTORS.key} less than " + | ||
| s"${DYN_ALLOCATION_MIN_EXECUTORS.key} is invalid, will use " + | ||
| s"${DYN_ALLOCATION_MIN_EXECUTORS} instead.") |
There was a problem hiding this comment.
so this doesn't cover is the user specified the executor instances < the minimum seems like we should also put out a warning for that also.
There was a problem hiding this comment.
also note that the comment above isn't exactly correct because if the number of executor instances is > then the min it would use that number instead of the min executors.
There was a problem hiding this comment.
I don't consider user specified executor instances, I'm not sure if we should also take this configuration into consideration.
There was a problem hiding this comment.
executor instances is what gets set if user puts in --num-executors which I see easily happening now and if min is already set and > then they are different and we will use min so we should warn here also.
There was a problem hiding this comment.
I agree that either spark.executor.instances or initialExecutors less than minimum should produce a warning.
|
Test build #62204 has finished for PR 14149 at commit
|
| if (conf.get(DYN_ALLOCATION_INITIAL_EXECUTORS) < conf.get(DYN_ALLOCATION_MIN_EXECUTORS)) { | ||
| logWarning(s"${DYN_ALLOCATION_INITIAL_EXECUTORS.key} less than " + | ||
| s"${DYN_ALLOCATION_MIN_EXECUTORS.key} is invalid, will use " + | ||
| s"${DYN_ALLOCATION_MIN_EXECUTORS.key} to override.") |
There was a problem hiding this comment.
so this isn't necessarily true, it could use num executor instances if that is higher then min. I think we should say using the max of other 2
There was a problem hiding this comment.
I think my original meaning is DYN_ALLOCATION_MIN_EXECUTORS will override DYN_ALLOCATION_INITIAL_EXECUTORS if the former is larger. Not related to num executor instance, num executor instance can still be larger than DYN_ALLOCATION_MIN_EXECUTORS.
There was a problem hiding this comment.
Right but as a user I could interpret that to mean its going to use the min executors for the initial number, which isn't necessarily true. I'm just worried it will confuse the users. How about we change the phrasing to be something like: initial less than min is invalid, ignoring its setting, please update your configs. Similar message for the num executor instances below .
Then after the max calculation we could simply print an info message like: using initial executors = value, max of initial, num executors, min executors.
These warnings are turning into a lot of extra code/text.
There was a problem hiding this comment.
That sounds good to me, let me update the patch.
|
Test build #62246 has finished for PR 14149 at commit
|
|
+1 |
…s than minExecutors ## What changes were proposed in this pull request? Currently if `spark.dynamicAllocation.initialExecutors` is less than `spark.dynamicAllocation.minExecutors`, Spark will automatically pick the minExecutors without any warning. While in 1.6 Spark will throw exception if configured like this. So here propose to add warning log if these parameters are configured invalidly. ## How was this patch tested? Unit test added to verify the scenario. Author: jerryshao <sshao@hortonworks.com> Closes #14149 from jerryshao/SPARK-16435. (cherry picked from commit d8220c1) Signed-off-by: Tom Graves <tgraves@yahoo-inc.com>
4 participants
What changes were proposed in this pull request?
Currently if
spark.dynamicAllocation.initialExecutorsis less thanspark.dynamicAllocation.minExecutors, Spark will automatically pick the minExecutors without any warning. While in 1.6 Spark will throw exception if configured like this. So here propose to add warning log if these parameters are configured invalidly.How was this patch tested?
Unit test added to verify the scenario.