[SPARK-42392][CORE] Add a new case of TriggeredByExecutorDecommissionInfo to remove unnecessary param#39962
[SPARK-42392][CORE] Add a new case of TriggeredByExecutorDecommissionInfo to remove unnecessary param#39962smallzhongfeng wants to merge 3 commits intoapache:masterfrom
TriggeredByExecutorDecommissionInfo to remove unnecessary param#39962Conversation
…o remove unnecessary param
smallzhongfeng
changed the title
|
Could you help me review this? @HyukjinKwon @LuciferYang @Ngone51 Thanks a lot. :-) |
smallzhongfeng
changed the title
TriggeredByExecutorDecommissionInfo to remove unnecessary param
|
Gentle ping. @LuciferYang @Ngone51 @dongjoon-hyun @HyukjinKwon :-) |
| abstract class DecommissionInfo | ||
|
|
||
| private[spark] | ||
| case class ExecutorDecommissionInfo( |
There was a problem hiding this comment.
hmm... Who triggered this? And I think we should add some comments for these two new classes
There was a problem hiding this comment.
Executor will triggered this. Maybe we can see this comment
There was a problem hiding this comment.
Could we also rename this one to TriggeredByXXXDecommissionInfo? Is it possible?
There was a problem hiding this comment.
Yes, how about ExecutorTriggerExecutorDecommissionInfo and DriverTriggerExecutorDecommissionInfo? Or could you give me a better opinion ?
There was a problem hiding this comment.
fine to me, it's good to be able to distinguish them clearly
| */ | ||
| private[spark] | ||
| case class ExecutorDecommissionInfo(message: String, workerHost: Option[String] = None) | ||
| abstract class DecommissionInfo |
There was a problem hiding this comment.
If all subclasses must to be defined in this file, maybe sealed?
There was a problem hiding this comment.
OK, sounds good!
| adjustTargetNumExecutors: Boolean): Boolean = { | ||
| val decommissionedExecutors = decommissionInfo match { | ||
| case _: ExecutorDecommissionInfo => | ||
| decommissionExecutors( |
There was a problem hiding this comment.
A little confused. Seems decommissionExecutors don't use triggeredByExecutor flag?
There was a problem hiding this comment.
Actually triggeredByExecutor was used in ExecutorAllocationClient 's implementation class CoarseGrainedSchedulerBackend and KubernetesClusterSchedulerBackend. We can see this
| package org.apache.spark.scheduler | ||
|
|
||
| private[spark] | ||
| sealed abstract class DecommissionInfo |
There was a problem hiding this comment.
abstract class? trait?
There was a problem hiding this comment.
It should not be necessary. After all, the case class here only needs to inherit this class DecommissionInfo. If the case class need multiple inheritance, it is more appropriate to use the trait here.
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
2 participants
What changes were proposed in this pull request?
For
TODO: add a new type like 'ExecutorDecommissionInfo' for the case where executor is decommissioned at executor first, so we don't need this extra parameter, I add a new case class named TriggeredByExecutorDecommissionInfo to resolve it.Why are the changes needed?
Remove unnecessary param.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Fix uts.