[GitHub] [spark] squito commented on a change in pull request #23951: [SPARK-27038][CORE][YARN] Re-implement RackResolver to reduce resolving time

[GitBox <gi...@apache.org>]

squito commented on a change in pull request #23951: [SPARK-27038][CORE][YARN] Re-implement RackResolver to reduce resolving time
URL: https://github.com/apache/spark/pull/23951#discussion_r263447342
 
 

 ##########
 File path: core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala
 ##########
 @@ -69,18 +69,48 @@ class FakeDAGScheduler(sc: SparkContext, taskScheduler: FakeTaskScheduler)
 // Get the rack for a given host
 object FakeRackUtil {
   private val hostToRack = new mutable.HashMap[String, String]()
+  var loopCount = 0
 
   def cleanUp() {
     hostToRack.clear()
+    loopCount = 0
   }
 
   def assignHostToRack(host: String, rack: String) {
     hostToRack(host) = rack
   }
 
   def getRackForHost(host: String): Option[String] = {
+    loopCount = simulateRunResolveCommand(Seq(host))
     hostToRack.get(host)
   }
+
+  def getRacksForHosts(hosts: List[String]): List[Option[String]] = {
+    loopCount = simulateRunResolveCommand(hosts)
+    hosts.map(hostToRack.get)
+  }
+
+  /**
+   * This is a simulation of building and executing the resolution command.
+   * Simulate function `runResolveCommand()` in [[org.apache.hadoop.net.ScriptBasedMapping]].
+   * If Seq has 100 elements, it returns 4. If Seq has 1 elements, it returns 1.
+   * @param args a list of arguments
+   * @return script execution times
+   */
+  private def simulateRunResolveCommand(args: Seq[String]): Int = {
+    val maxArgs = 30 // Simulate NET_TOPOLOGY_SCRIPT_NUMBER_ARGS_DEFAULT
+    var numProcessed = 0
+    var loopCount = 0
+    while (numProcessed != args.size) {
+      var start = maxArgs * loopCount
+      numProcessed = start
+      while (numProcessed < (start + maxArgs) && numProcessed < args.size) {
+        numProcessed += 1
+      }
+      loopCount += 1
+    }
+    loopCount
 
 Review comment:
   is there really any value in adding this complexity to the test?
   
   Stepping back a bit, I think there are 2 changes you're making here to improve performance:
   
   1) you use `getRacksForHosts` (plural) to requests the racks in bulk
   
   2) you are changing the way the rack resolver itself works, so that it will cache requests for the same host.
   
   3) If spark.locality.rack.wait == 0, you skip rack resolution entirely.
   
   I think here you just want to test (1), and for that, you just need to count invocations for `getRackForHost()` vs. counts of `getRacksForHosts()`
   
   Would it be worth also adding a test for (2) somehow?  I'm not sure what you could do there without it being tied to the internals of the hadoop logic.   You could test the instance of the created 
   
   Also I think it would be easy and useful for you to add a test for (3) here, the same way you're testing (1), just that you'd expect `getRacksForHosts()` to be called 0 times.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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