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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
