You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "shrinidhijoshi (via GitHub)" <gi...@apache.org> on 2023/11/17 16:18:34 UTC
[PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
shrinidhijoshi opened a new pull request, #43873:
URL: https://github.com/apache/spark/pull/43873
### What changes were proposed in this pull request?
Renames the taskScheduler reference variable from `scheduler` to `taskScheduler`
### Why are the changes needed?
CoarseGrainedScheduler currently holds reference to TaskScheduler in a variable `scheduler`.
This is confusing when reading scheduler code. This PR renames the local variable to `taskScheduler`
to 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398000322
##########
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala:
##########
@@ -52,14 +52,14 @@ import org.apache.spark.util.ArrayImplicits._
* Spark's standalone deploy mode (spark.deploy.*).
*/
private[spark]
-class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: RpcEnv)
+class CoarseGrainedSchedulerBackend(taskScheduler: TaskSchedulerImpl, val rpcEnv: RpcEnv)
Review Comment:
ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #43873: [MINOR][CORE] Rename scheduler ref for readability
URL: https://github.com/apache/spark/pull/43873
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1397999938
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -75,7 +75,7 @@ private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
override val rpcEnv: RpcEnv = sc.env.rpcEnv
- private[spark] var scheduler: TaskScheduler = null
+ private[spark] var taskScheduler: TaskScheduler = null
Review Comment:
Thank you for making a PR, but I don't think we need this change inside `HeartbeatReceiver`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398000741
##########
core/src/test/scala/org/apache/spark/HeartbeatReceiverSuite.scala:
##########
@@ -97,9 +97,9 @@ class HeartbeatReceiverSuite
}
test("task scheduler is set correctly") {
- assert(heartbeatReceiver.scheduler === null)
+ assert(heartbeatReceiver.taskScheduler === null)
Review Comment:
ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43873:
URL: https://github.com/apache/spark/pull/43873#issuecomment-1965565810
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.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398020821
##########
core/src/main/scala/org/apache/spark/scheduler/TaskResultGetter.scala:
##########
@@ -32,7 +32,7 @@ import org.apache.spark.util.{LongAccumulator, ThreadUtils, Utils}
/**
* 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)
Review Comment:
Personally, the `scheduler` looks more minimalist here. No one would suspect that it is related to a task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398020821
##########
core/src/main/scala/org/apache/spark/scheduler/TaskResultGetter.scala:
##########
@@ -32,7 +32,7 @@ import org.apache.spark.util.{LongAccumulator, ThreadUtils, Utils}
/**
* 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)
Review Comment:
Traditionally, the `scheduler` looks more minimalist here. No one would suspect that it is related to a task.
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -49,19 +49,19 @@ import org.apache.spark.util.collection.PercentileHeap
* THREADING: This class is designed to only be called from code with a lock on the
* TaskScheduler (e.g. its event handlers). It should not be called from other threads.
*
- * @param sched the TaskSchedulerImpl associated with the TaskSetManager
+ * @param taskScheduler the TaskSchedulerImpl associated with the TaskSetManager
* @param taskSet the TaskSet to manage scheduling for
* @param maxTaskFailures if any particular task fails this number of times, the entire
* task set will be aborted
*/
private[spark] class TaskSetManager(
- sched: TaskSchedulerImpl,
+ taskScheduler: TaskSchedulerImpl,
Review Comment:
I suggest `sched` -> `scheduler`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398000582
##########
core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala:
##########
@@ -41,10 +41,10 @@ import org.apache.spark.util.{ThreadUtils, Utils}
* A [[SchedulerBackend]] implementation for Spark's standalone cluster manager.
*/
private[spark] class StandaloneSchedulerBackend(
- scheduler: TaskSchedulerImpl,
+ taskScheduler: TaskSchedulerImpl,
Review Comment:
A wrong indentation. Also, we don't need this change in this class context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398000121
##########
core/src/main/scala/org/apache/spark/scheduler/TaskResultGetter.scala:
##########
@@ -32,7 +32,7 @@ import org.apache.spark.util.{LongAccumulator, ThreadUtils, Utils}
/**
* 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)
Review Comment:
ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][CORE] Rename scheduler ref for readability [spark]
Posted by "shrinidhijoshi (via GitHub)" <gi...@apache.org>.
shrinidhijoshi commented on code in PR #43873:
URL: https://github.com/apache/spark/pull/43873#discussion_r1398018102
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -75,7 +75,7 @@ private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
override val rpcEnv: RpcEnv = sc.env.rpcEnv
- private[spark] var scheduler: TaskScheduler = null
+ private[spark] var taskScheduler: TaskScheduler = null
Review Comment:
Sure @dongjoon-hyun. I am curious to understand why you suggest it is not needed ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org