uros-b commented on code in PR #57257:
URL: https://github.com/apache/spark/pull/57257#discussion_r3672615149
##########
core/src/main/scala/org/apache/spark/scheduler/Schedulable.scala:
##########
@@ -21,16 +21,19 @@ import java.util.concurrent.ConcurrentLinkedQueue
import scala.collection.mutable.ArrayBuffer
+import org.apache.spark.annotation.DeveloperApi
import org.apache.spark.scheduler.SchedulingMode.SchedulingMode
/**
- * An interface for schedulable entities.
- * there are two type of Schedulable entities(Pools and TaskSetManagers)
+ * :: DeveloperApi ::
+ * An interface for schedulable entities (there are two types: Pools and
TaskSetManagers).
+ *
+ * Only the read-only scheduling properties below are part of the public API
surface; they are the
+ * inputs a custom [[SchedulingAlgorithm]] can inspect in its `comparator`.
The structural and
+ * mutating members are Spark-internal (`private[spark]`) and must not be used
by applications.
*/
-private[spark] trait Schedulable {
- var parent: Pool
- // child queues
- def schedulableQueue: ConcurrentLinkedQueue[Schedulable]
+@DeveloperApi
Review Comment:
I'd say `SchedulableInfo` is a better option than `Schedulable` here.
- As declared here, the trait can't actually be implemented outside
`org.apache.spark`; every structural member is abstract *and* `private[spark]`,
so no external class can satisfy it. The only capability the public trait
grants is *reading* seven properties, while permanently pinning the trait's
identity and member set as API that MiMa will enforce.
- It's out of line with what's already `@DeveloperApi` in this package.
`TaskInfo`, `StageInfo`, `AccumulableInfo`, `ExecutorInfo` are all immutable
value/info types. `Schedulable` is a live node in a mutable hierarchy:
`Pool.runningTasks` and `stageId` are plain `var`s mutated from scheduler
threads via `increaseRunningTasks`, so a user who stashes a `Schedulable`
reference is reading unsynchronized mutable state. The values are also hard to
use without internal knowledge (`priority` is the job id, `stageId` is `-1` for
pools, `minShare`/`weight` only mean something in FAIR mode).
An immutable `SchedulableInfo` snapshot passed to the comparator solves
both: `Schedulable` and `Pool` stay fully internal and free to evolve, and the
public surface is a documented value type consistent with the rest of the
package. The extra class seems a small price. While shaping it, worth
considering whether the callback should be an
`Ordering`/`Comparator[SchedulableInfo]` rather than a bare boolean `lt`, and
renaming `s1`/`s2` to something like `left`/`right`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
