[GitHub] [spark] squito commented on a change in pull request #24935: [SPARK-28005][YARN] Remove unnecessary log from SparkRackResolver

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

squito commented on a change in pull request #24935: [SPARK-28005][YARN] Remove unnecessary log from SparkRackResolver
URL: https://github.com/apache/spark/pull/24935#discussion_r296788625
 
 

 ##########
 File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/SparkRackResolver.scala
 ##########
 @@ -70,18 +70,20 @@ private[spark] class SparkRackResolver(conf: Configuration) extends Logging {
     val nodes = new ArrayBuffer[Node]
     // dnsToSwitchMapping is thread-safe
     val rNameList = dnsToSwitchMapping.resolve(hostNames.toList.asJava).asScala
-    if (rNameList == null || rNameList.isEmpty) {
+    if (rNameList == null) {
       hostNames.foreach(nodes += new NodeBase(_, NetworkTopology.DEFAULT_RACK))
       logInfo(s"Got an error when resolving hostNames. " +
         s"Falling back to ${NetworkTopology.DEFAULT_RACK} for all")
     } else {
-      for ((hostName, rName) <- hostNames.zip(rNameList)) {
-        if (Strings.isNullOrEmpty(rName)) {
-          nodes += new NodeBase(hostName, NetworkTopology.DEFAULT_RACK)
-          logDebug(s"Could not resolve $hostName. " +
-            s"Falling back to ${NetworkTopology.DEFAULT_RACK}")
-        } else {
-          nodes += new NodeBase(hostName, rName)
+      if (rNameList.nonEmpty) {
+        for ((hostName, rName) <- hostNames.zip(rNameList)) {
 
 Review comment:
   if `hostNames` is nonEmpty, but `rNameList` is empty, this will just silently skip everything, right?  that shouldn't happen, but as we're calling code outside of spark a little defensiveness would be good here.  I think the calling code requires the returned list to be the same length as `hostNames`.
   
   I am also biased against early returns, but here I think it really is the simplest alternative.
   
   (sorry for so much quibbling about such a small thing ...)

----------------------------------------------------------------
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