[MINOR][CORE] Rename scheduler ref for readability#43873
[MINOR][CORE] Rename scheduler ref for readability#43873shrinidhijoshi wants to merge 1 commit intoapache:masterfrom
Conversation
shrinidhijoshi
force-pushed
the
renameRef
branch
from
83d3f1e to
0138940
Compare
shrinidhijoshi
force-pushed
the
renameRef
branch
from
0138940 to
1cd24f6
Compare
| override val rpcEnv: RpcEnv = sc.env.rpcEnv | ||
|
|
||
| private[spark] var scheduler: TaskScheduler = null | ||
| private[spark] var taskScheduler: TaskScheduler = null |
There was a problem hiding this comment.
Thank you for making a PR, but I don't think we need this change inside HeartbeatReceiver.
There was a problem hiding this comment.
Sure @dongjoon-hyun. I am curious to understand why you suggest it is not needed ?
| * Runs a thread pool that deserializes and remotely fetches (if necessary) task results. | ||
| */ | ||
| private[spark] class TaskResultGetter(sparkEnv: SparkEnv, scheduler: TaskSchedulerImpl) | ||
| private[spark] class TaskResultGetter(sparkEnv: SparkEnv, taskScheduler: TaskSchedulerImpl) |
There was a problem hiding this comment.
Personally, the scheduler looks more minimalist here. No one would suspect that it is related to a task.
| */ | ||
| private[spark] | ||
| class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: RpcEnv) | ||
| class CoarseGrainedSchedulerBackend(taskScheduler: TaskSchedulerImpl, val rpcEnv: RpcEnv) |
| */ | ||
| private[spark] class StandaloneSchedulerBackend( | ||
| scheduler: TaskSchedulerImpl, | ||
| taskScheduler: TaskSchedulerImpl, |
There was a problem hiding this comment.
A wrong indentation. Also, we don't need this change in this class context.
|
|
||
| test("task scheduler is set correctly") { | ||
| assert(heartbeatReceiver.scheduler === null) | ||
| assert(heartbeatReceiver.taskScheduler === null) |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
The following change looks legit from my perspective. WDYT, @shrinidhijoshi ?
private[spark] class TaskSetManager(
- sched: TaskSchedulerImpl,
+ taskScheduler: TaskSchedulerImpl,
| * Runs a thread pool that deserializes and remotely fetches (if necessary) task results. | ||
| */ | ||
| private[spark] class TaskResultGetter(sparkEnv: SparkEnv, scheduler: TaskSchedulerImpl) | ||
| private[spark] class TaskResultGetter(sparkEnv: SparkEnv, taskScheduler: TaskSchedulerImpl) |
There was a problem hiding this comment.
Personally, the scheduler looks more minimalist here. No one would suspect that it is related to a task.
| */ | ||
| private[spark] class TaskSetManager( | ||
| sched: TaskSchedulerImpl, | ||
| taskScheduler: TaskSchedulerImpl, |
There was a problem hiding this comment.
I suggest sched -> scheduler.
|
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. |
3 participants
What changes were proposed in this pull request?
Renames (standardize) the
TaskSchedulerreference variables fromscheduler/schedetc totaskSchedulerWhy are the changes needed?
Multiple classes in
core/( likeCoarseGrainedScheduler,StandaloneScheduler, etc) currently hold reference to TaskScheduler in a variablescheduler/sched, etc. This is confusing when reading scheduler code. This PR renames these reference variables totaskSchedulerto make the code more readable.Does this PR introduce any user-facing change?
No
How was this patch tested?
Github Actions
Was this patch authored or co-authored using generative AI tooling?
No