[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-24 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r296715635
 
 

 ##
 File path: core/src/main/scala/org/apache/spark/internal/config/Worker.scala
 ##
 @@ -20,6 +20,17 @@ package org.apache.spark.internal.config
 import java.util.concurrent.TimeUnit
 
 private[spark] object Worker {
+  val SPARK_WORKER_PREFIX = "spark.worker"
+
+  val SPARK_WORKER_RESOURCE_FILE =
+ConfigBuilder("spark.worker.resourcesFile")
+.internal()
+.doc("Path to a file containing the resources allocated to the worker. " +
+  "The file should be formatted as a JSON array of ResourceInformation 
objects. " +
 
 Review comment:
   I don't think this is true anymore, I think the json has to be formatted 
like a ResourceAllocation


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-13 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r293452251
 
 

 ##
 File path: core/src/test/scala/org/apache/spark/deploy/worker/WorkerSuite.scala
 ##
 @@ -60,7 +60,7 @@ class WorkerSuite extends SparkFunSuite with Matchers with 
BeforeAndAfter {
 val securityMgr = new SecurityManager(conf)
 val rpcEnv = RpcEnv.create("test", "localhost", 12345, conf, securityMgr)
 _worker = new Worker(rpcEnv, 5, 20, 1234 * 5, 
Array.fill(1)(RpcAddress("1.2.3.4", 1234)),
-  "Worker", "/tmp", conf, securityMgr, shuffleServiceSupplier)
+  "Worker", "/tmp", conf, securityMgr, None, Map.empty, 
shuffleServiceSupplier)
 
 Review comment:
   sounds fine


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292931285
 
 

 ##
 File path: core/src/test/scala/org/apache/spark/deploy/worker/WorkerSuite.scala
 ##
 @@ -60,7 +60,7 @@ class WorkerSuite extends SparkFunSuite with Matchers with 
BeforeAndAfter {
 val securityMgr = new SecurityManager(conf)
 val rpcEnv = RpcEnv.create("test", "localhost", 12345, conf, securityMgr)
 _worker = new Worker(rpcEnv, 5, 20, 1234 * 5, 
Array.fill(1)(RpcAddress("1.2.3.4", 1234)),
-  "Worker", "/tmp", conf, securityMgr, shuffleServiceSupplier)
+  "Worker", "/tmp", conf, securityMgr, None, Map.empty, 
shuffleServiceSupplier)
 
 Review comment:
   if its easier to do with the next jira for assigments that is fine too


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292916724
 
 

 ##
 File path: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
 ##
 @@ -220,6 +225,38 @@ private[deploy] class Worker(
 metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
   }
 
+  // TODO if we're starting up multi workers under the same host, discovery 
script won't work.
+  private def setupWorkerResources(): Unit = {
+try {
+  resources = resourceFile.map { rFile =>
+ResourceDiscoverer.parseAllocatedFromJsonFile(rFile)
+  }.getOrElse {
+if (resourceDiscoveryScript.isEmpty) {
+  logWarning(s"Neither resourceFile nor resourceDiscoveryScript found 
for worker. " +
+s"You can use SPARK_WORKER_RESOURCE_FILE(--resource-file) or " +
+s"SPARK_WORKER_RESOURCE_DISCOVERY_SCRIPT(--resource-script) to 
config " +
+s"resources(e.g. GPU/FPGA) for worker.")
+  Map.empty
+} else {
+  resourceDiscoveryScript.map { case (rName, rScript) =>
+val resInfo = ResourceDiscoverer.getResourceInfo(rScript, rName,
+  "SPARK_WORKER_RESOURCE_DISCOVERY_SCRIPT or (--resource-script)")
+(rName, resInfo)
+  }
+}
+  }
+} catch {
+  case t: Throwable =>
 
 Review comment:
   is there a reason we catch all Throwable here?  I think just Exception makes 
more sense.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292917499
 
 

 ##
 File path: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
 ##
 @@ -220,6 +225,38 @@ private[deploy] class Worker(
 metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
   }
 
+  // TODO if we're starting up multi workers under the same host, discovery 
script won't work.
 
 Review comment:
   technically a discovery script could work but the writer of that script 
would have to handle that case inside of the script to assign each worker 
different resources.  Either way we should be sure to document it 


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292913851
 
 

 ##
 File path: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
 ##
 @@ -220,6 +225,38 @@ private[deploy] class Worker(
 metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
   }
 
+  // TODO if we're starting up multi workers under the same host, discovery 
script won't work.
+  private def setupWorkerResources(): Unit = {
+try {
+  resources = resourceFile.map { rFile =>
+ResourceDiscoverer.parseAllocatedFromJsonFile(rFile)
+  }.getOrElse {
+if (resourceDiscoveryScript.isEmpty) {
+  logWarning(s"Neither resourceFile nor resourceDiscoveryScript found 
for worker. " +
+s"You can use SPARK_WORKER_RESOURCE_FILE(--resource-file) or " +
+s"SPARK_WORKER_RESOURCE_DISCOVERY_SCRIPT(--resource-script) to 
config " +
+s"resources(e.g. GPU/FPGA) for worker.")
+  Map.empty
+} else {
+  resourceDiscoveryScript.map { case (rName, rScript) =>
+val resInfo = ResourceDiscoverer.getResourceInfo(rScript, rName,
+  "SPARK_WORKER_RESOURCE_DISCOVERY_SCRIPT or (--resource-script)")
+(rName, resInfo)
+  }
+}
+  }
+} catch {
+  case t: Throwable =>
+logWarning("Failed to setup worker resources: ", t)
 
 Review comment:
   logError


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292912943
 
 

 ##
 File path: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
 ##
 @@ -220,6 +225,38 @@ private[deploy] class Worker(
 metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
   }
 
+  // TODO if we're starting up multi workers under the same host, discovery 
script won't work.
+  private def setupWorkerResources(): Unit = {
+try {
+  resources = resourceFile.map { rFile =>
+ResourceDiscoverer.parseAllocatedFromJsonFile(rFile)
+  }.getOrElse {
+if (resourceDiscoveryScript.isEmpty) {
+  logWarning(s"Neither resourceFile nor resourceDiscoveryScript found 
for worker. " +
 
 Review comment:
   I'm not sure you want to warn here.  If users just aren't configuring spark 
to use other resources there is no reason to warn.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292923079
 
 

 ##
 File path: core/src/test/scala/org/apache/spark/deploy/worker/WorkerSuite.scala
 ##
 @@ -60,7 +60,7 @@ class WorkerSuite extends SparkFunSuite with Matchers with 
BeforeAndAfter {
 val securityMgr = new SecurityManager(conf)
 val rpcEnv = RpcEnv.create("test", "localhost", 12345, conf, securityMgr)
 _worker = new Worker(rpcEnv, 5, 20, 1234 * 5, 
Array.fill(1)(RpcAddress("1.2.3.4", 1234)),
-  "Worker", "/tmp", conf, securityMgr, shuffleServiceSupplier)
+  "Worker", "/tmp", conf, securityMgr, None, Map.empty, 
shuffleServiceSupplier)
 
 Review comment:
   can we add more tests that actually get the resources?


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292922457
 
 

 ##
 File path: 
core/src/main/scala/org/apache/spark/deploy/worker/WorkerArguments.scala
 ##
 @@ -36,6 +36,8 @@ private[worker] class WorkerArguments(args: Array[String], 
conf: SparkConf) {
   var memory = inferDefaultMemory()
   var masters: Array[String] = null
   var workDir: String = null
+  var resourceFile: Option[String] = None
+  var resourceDiscoveryScript: Map[String, String] = Map()
 
 Review comment:
   = Map.empty might be more clear that this is immutable.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] Setup resources when Standalone Worker starts up

2019-06-12 Thread GitBox
tgravescs commented on a change in pull request #24841: [SPARK-27369][CORE] 
Setup resources when Standalone Worker starts up
URL: https://github.com/apache/spark/pull/24841#discussion_r292920997
 
 

 ##
 File path: 
core/src/main/scala/org/apache/spark/deploy/worker/WorkerArguments.scala
 ##
 @@ -54,6 +56,12 @@ private[worker] class WorkerArguments(args: Array[String], 
conf: SparkConf) {
   if (System.getenv("SPARK_WORKER_DIR") != null) {
 workDir = System.getenv("SPARK_WORKER_DIR")
   }
+  if (System.getenv("SPARK_WORKER_RESOURCE_FILE") != null) {
+resourceFile = Some(System.getenv("SPARK_WORKER_RESOURCE_FILE"))
+  }
+  if (System.getenv("SPARK_WORKER_RESOURCE_DISCOVERY_SCRIPT") != null) {
+
parseResourceDiscoveryScript(conf.getenv("SPARK_WORKER_RESOURCE_DISCOVERY_SCRIPT"))
 
 Review comment:
   might be easier to read if we have parseResourceDiscoveryScript return a Map 
and set it here. 


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org