[GitHub] [spark] xkrogen commented on a change in pull request #29906: [SPARK-32037][CORE] Rename blacklisting feature

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

xkrogen commented on a change in pull request #29906:
URL: https://github.com/apache/spark/pull/29906#discussion_r497085607



##########
File path: core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala
##########
@@ -381,7 +381,7 @@ private[spark] class TaskSchedulerImpl(
     : (Boolean, Option[TaskLocality]) = {
     var noDelayScheduleRejects = true
     var minLaunchedLocality: Option[TaskLocality] = None
-    // nodes and executors that are blacklisted for the entire application have already been
+    // nodes and executors that are blocked for the entire application have already been

Review comment:
       excluded?

##########
File path: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedClusterMessage.scala
##########
@@ -122,7 +122,7 @@ private[spark] object CoarseGrainedClusterMessages {
       resourceProfileToTotalExecs: Map[ResourceProfile, Int],
       numLocalityAwareTasksPerResourceProfileId: Map[Int, Int],
       hostToLocalTaskCount: Map[Int, Map[String, Int]],
-      nodeBlacklist: Set[String])
+      nodeBlocklist: Set[String])

Review comment:
       nodeExcludelist?

##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnAllocatorSuite.scala
##########
@@ -523,9 +523,9 @@ class YarnAllocatorSuite extends SparkFunSuite with Matchers with BeforeAndAfter
     handler.getNumUnexpectedContainerRelease should be (2)
   }
 
-  test("blacklisted nodes reflected in amClient requests") {
-    // Internally we track the set of blacklisted nodes, but yarn wants us to send *changes*
-    // to the blacklist.  This makes sure we are sending the right updates.
+  test("excludeOnFailure nodes reflected in amClient requests") {
+    // Internally we track the set of excluded nodes, but yarn wants us to send *changes*
+    // to it.  This makes sure we are sending the right updates.

Review comment:
       I don't think the changes in `YarnAllocatorSuite` are correct -- I think this is testing the interaction with YARN's blacklisting feature (e.g. YARN-4576), rather than Spark's feature. I think it's the same for `YarnSchedulerBackendSuite` ?

##########
File path: sql/hive-thriftserver/v1.2/src/main/java/org/apache/hive/service/auth/HiveAuthFactory.java
##########
@@ -291,14 +291,14 @@ public static TServerSocket getServerSSLSocket(String hiveHost, int portNum, Str
     TServerSocket thriftServerSocket =
         TSSLTransportFactory.getServerSocket(portNum, 0, serverAddress.getAddress(), params);
     if (thriftServerSocket.getServerSocket() instanceof SSLServerSocket) {
-      List<String> sslVersionBlacklistLocal = new ArrayList<String>();
-      for (String sslVersion : sslVersionBlacklist) {
-        sslVersionBlacklistLocal.add(sslVersion.trim().toLowerCase(Locale.ROOT));
+      List<String> sslVersionsExcludedLocal = new ArrayList<String>();
+      for (String sslVersion : sslVersionsExcluded) {
+        sslVersionsExcludedLocal.add(sslVersion.trim().toLowerCase(Locale.ROOT));
       }
       SSLServerSocket sslServerSocket = (SSLServerSocket) thriftServerSocket.getServerSocket();
       List<String> enabledProtocols = new ArrayList<String>();
       for (String protocol : sslServerSocket.getEnabledProtocols()) {
-        if (sslVersionBlacklistLocal.contains(protocol.toLowerCase(Locale.ROOT))) {
+        if (sslVersionsExcludedLocal.contains(protocol.toLowerCase(Locale.ROOT))) {

Review comment:
       Changes here (and in the other `sql/hive-thriftserver` files) shouldn't be included in this PR. Previously I was suggested not to adjust these since they are a direct copy of Hive files -- and either way it's not relevant to the excludeOnFailure feature.

##########
File path: core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala
##########
@@ -515,15 +515,15 @@ private[spark] class TaskSchedulerImpl(
       hostsByRack.getOrElseUpdate(rack, new HashSet[String]()) += host
     }
 
-    // Before making any offers, remove any nodes from the blacklist whose blacklist has expired. Do
+    // Before making any offers, remove any nodes from the blocklist whose blocklist has expired. Do
     // this here to avoid a separate thread and added synchronization overhead, and also because
-    // updating the blacklist is only relevant when task offers are being made.
-    blacklistTrackerOpt.foreach(_.applyBlacklistTimeout())
+    // updating the blocklist is only relevant when task offers are being made.

Review comment:
       blocklist -> excludeList?

##########
File path: core/src/main/scala/org/apache/spark/status/AppStatusSource.scala
##########
@@ -59,9 +59,17 @@ private[spark] class AppStatusSource extends Source {
 
   val SKIPPED_TASKS = getCounter("tasks", "skippedTasks")
 
+  // this is private but user visible from metrics so just deprecate
+  // @deprecated("use excludedExecutors instead", "3.1.0")

Review comment:
       Why is the deprecation commented out?




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



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