[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677117650 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskDescription.scala ## @@ -57,6 +57,7 @@ private[spark] class TaskDescription( val addedJars: Map[String, Long], val addedArchives: Map[String, Long], val properties: Properties, +val cpus: Int, Review comment: @xwu99 Please add a check to make sure we dont have invalid value for `cpus` coming in - `assert(cpus > 0)`. There should be some validation either here, or where it is getting passed in from. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677116925 ## File path: core/src/main/scala/org/apache/spark/TaskContextImpl.scala ## @@ -54,6 +54,7 @@ private[spark] class TaskContextImpl( @transient private val metricsSystem: MetricsSystem, // The default value is only used in tests. override val taskMetrics: TaskMetrics = TaskMetrics.empty, +override val cpus: Int = 0, Review comment: @tgravescs For default value, CPUS_PER_TASK is simply "spark.task.cpus" with default 1 ... which should be available from `SparkEnv.conf` ? `SparkEnv.conf.get(config.CPUS_PER_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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677116925 ## File path: core/src/main/scala/org/apache/spark/TaskContextImpl.scala ## @@ -54,6 +54,7 @@ private[spark] class TaskContextImpl( @transient private val metricsSystem: MetricsSystem, // The default value is only used in tests. override val taskMetrics: TaskMetrics = TaskMetrics.empty, +override val cpus: Int = 0, Review comment: @tgravescs CPUS_PER_TASK is simply "spark.task.cpus" with default 1 ... which should be available from `SparkEnv.conf` ? `SparkEnv.conf.get(config.CPUS_PER_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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677117650 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskDescription.scala ## @@ -57,6 +57,7 @@ private[spark] class TaskDescription( val addedJars: Map[String, Long], val addedArchives: Map[String, Long], val properties: Properties, +val cpus: Int, Review comment: Please add a check to make sure we dont have invalid value for `cpus` coming in - `assert(cpus > 0)`. There should be some validation either here, or where it is getting passed in from. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677117650 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskDescription.scala ## @@ -57,6 +57,7 @@ private[spark] class TaskDescription( val addedJars: Map[String, Long], val addedArchives: Map[String, Long], val properties: Properties, +val cpus: Int, Review comment: Please add a check to make sure we dont have invalid value for `cpus` coming in - `assert(cpus > 0)`. Validation either here, or where it is getting set from. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677117650 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskDescription.scala ## @@ -57,6 +57,7 @@ private[spark] class TaskDescription( val addedJars: Map[String, Long], val addedArchives: Map[String, Long], val properties: Properties, +val cpus: Int, Review comment: Please add a check to make sure we dont have invalid value for `cpus` coming in - `assert(cpus > 0)`. Some form of validation either here, or where it is getting set from. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677117650 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskDescription.scala ## @@ -57,6 +57,7 @@ private[spark] class TaskDescription( val addedJars: Map[String, Long], val addedArchives: Map[String, Long], val properties: Properties, +val cpus: Int, Review comment: Please add a check to make sure we dont have invalid value for `cpus` coming in - `assert(cpus > 0)` -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677116925 ## File path: core/src/main/scala/org/apache/spark/TaskContextImpl.scala ## @@ -54,6 +54,7 @@ private[spark] class TaskContextImpl( @transient private val metricsSystem: MetricsSystem, // The default value is only used in tests. override val taskMetrics: TaskMetrics = TaskMetrics.empty, +override val cpus: Int = 0, Review comment: CPUS_PER_TASK is simply "spark.task.cpus" with default 1 ... which should be available from `SparkEnv.conf` ? `SparkEnv.conf.get(config.CPUS_PER_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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r677115269 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala ## @@ -429,6 +429,7 @@ private[spark] class TaskSetManager( execId: String, host: String, maxLocality: TaskLocality.TaskLocality, + taskCpuAssignments: Int = 1, Review comment: If we can require explicitly passing it, that would be ideal, I agree. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r674239571 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala ## @@ -429,6 +429,7 @@ private[spark] class TaskSetManager( execId: String, host: String, maxLocality: TaskLocality.TaskLocality, + taskCpuAssignments: Int = 1, Review comment: Can we not default to `CPUS_PER_TASK` @tgravescs, which is current behavior ? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] mridulm commented on a change in pull request #33385: [SPARK-36173][CORE] Support getting CPU number in TaskContext
mridulm commented on a change in pull request #33385: URL: https://github.com/apache/spark/pull/33385#discussion_r674241685 ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskDescription.scala ## @@ -57,6 +57,7 @@ private[spark] class TaskDescription( val addedJars: Map[String, Long], val addedArchives: Map[String, Long], val properties: Properties, +val cpus: Int, Review comment: Add a precondition for `cpus` > 0 ? ## File path: core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala ## @@ -429,6 +429,7 @@ private[spark] class TaskSetManager( execId: String, host: String, maxLocality: TaskLocality.TaskLocality, + taskCpuAssignments: Int = 1, Review comment: Can we not default to `CPUS_PER_TASK` which is current behavior ? ## File path: core/src/main/scala/org/apache/spark/TaskContextImpl.scala ## @@ -54,6 +54,7 @@ private[spark] class TaskContextImpl( @transient private val metricsSystem: MetricsSystem, // The default value is only used in tests. override val taskMetrics: TaskMetrics = TaskMetrics.empty, +override val cpus: Int = 0, Review comment: Default to CPUS_PER_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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
