Skip to content

Conversation

@ulysses-you
Copy link
Contributor

Why are the changes needed?

This pr adds a new rule FinalStageResourceManager to eagerly kill redundant executors

We first get the final stage partition which is the actually required cores, then kill the redundant executors. The priority of kill executors follow:

  1. kill executor who is younger than other (The older the JIT works better)
  2. kill executor who produces less shuffle data first

The reason why add this feature is that, if the previous stage contains lots executors but final stage has less, then the tasks of final stage would be scheduled randomly in all exists executors which may cause resource waste. e.g., each executor only run 1 or 2 tasks but holds 4 or 5 cores.

How was this patch tested?

test manually

  • test for the kill executor

image

@ulysses-you
Copy link
Contributor Author

.createWithDefault(true)

val FINAL_WRITE_STAGE_EAGERLY_KILL_EXECUTORS_ENABLED =
buildConf("spark.sql.finalWriteStageEagerlyKillExecutors.enabled")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's valuable to introduce a new namespace spark.sql.finalWriteStage.

Suggested change
buildConf("spark.sql.finalWriteStageEagerlyKillExecutors.enabled")
buildConf("spark.sql.finalWriteStage.eagerlyKillExecutors.enabled")

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2023

Codecov Report

Merging #4592 (28d4230) into master (351bab3) will increase coverage by 0.03%.
The diff coverage is 90.90%.

❗ Current head 28d4230 differs from pull request most recent head f35208b. Consider uploading reports for the commit f35208b to get more accurate results

@@             Coverage Diff              @@
##             master    #4592      +/-   ##
============================================
+ Coverage     53.26%   53.30%   +0.03%     
  Complexity       13       13              
============================================
  Files           577      577              
  Lines         31557    31568      +11     
  Branches       4244     4245       +1     
============================================
+ Hits          16810    16827      +17     
+ Misses        13161    13153       -8     
- Partials       1586     1588       +2     
Impacted Files Coverage Δ
...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala 98.37% <90.90%> (-0.74%) ⬇️

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

// - target executors > min executors
val numActiveExecutors = sc.getExecutorIds().length
val expectedCores = partitionSpecs.length
val targetExecutors = (((expectedCores / executorCores) + 1) * factor).toInt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(expectedCores / executorCores) + 1 => math.ceil(expectedCores.toFloat / executorCores)

// - target executors > min executors
val numActiveExecutors = sc.getExecutorIds().length
val expectedCores = partitionSpecs.length
val targetExecutors = (((expectedCores / executorCores) + 1) * factor).toInt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep at least 1 exec?

executorIdsToKill.toSeq
}

private def killExecutors(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org.apache.spark.SparkContext#killExecutors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a story about DRA. Since apache/spark#20604, org.apache.spark.SparkContext#killExecutors does not allow with DRA ON, so this pr hack the internal interface to kill executors. I think that pr is not very reaonable, it should be ok to kill executors with DRA ON if the min executor is less than the target executor.

@yaooqinn
Copy link
Member

kill executor who is younger than other (The older the JIT works better)

This is not always true,

  • the generated code may not JITed in the final stage.
  • The old ones are very like to overloaded in some cases
  • Potential Problems happen with shuffle tracking?

*/
case class FinalStageResourceManager(session: SparkSession) extends Rule[SparkPlan] {
override def apply(plan: SparkPlan): SparkPlan = {
if (!conf.getConf(KyuubiSQLConf.FINAL_WRITE_STAGE_EAGERLY_KILL_EXECUTORS_ENABLED)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dynamicAllocation enabled?

@ulysses-you
Copy link
Contributor Author

@yaooqinn

the generated code may not JITed in the final stage.

yea, but at least, the JIT should work better for shuffle read and scheduler related code with the older one

the old ones are very like to overloaded in some cases

it really depend on the machine .. I'm not sure how to find the bad machine in a easy way

Potential Problems happen with shuffle tracking?

We first kill executor according to the alive time and exclude the one who has shuffle data, so it should be safe for this case.

* limitations under the License.
*/

package org.apache.spark
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for unstable calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, e.g., CoarseGrainedSchedulerBackend is under private[spark]

@ulysses-you
Copy link
Contributor Author

thanks for review, merging to master !

@ulysses-you ulysses-you deleted the eagerly-kill-executors branch March 24, 2023 10:25
@ulysses-you ulysses-you added this to the v1.8.0 milestone Mar 24, 2023
@ulysses-you ulysses-you self-assigned this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants