[SPARK-13371] [CORE] TaskSetManager.dequeueSpeculativeTask compares Option[String] and String directly. #11253
Conversation
… in at least one case a likely bug (TaskSetManager)
|
|
||
| // Test whether dependencies have been changed from its earlier parent RDD | ||
| assert(operatedRDD.dependencies.head.rdd != parentRDD) | ||
| assert(operatedRDD.dependencies.head != parentRDD) |
There was a problem hiding this comment.
Please fix the original misnomer that encouraged this bad comparison: parentRDD is not an RDD, but rather a Dependency.
|
What are the implications of this bug? We never schedule anything rack local? |
|
Good call, will make that change. Eh, yeah I'm not sure what the implication is but it doesn't sound great. Well, fix going in for sure. |
| // Check for rack-local tasks | ||
| if (TaskLocality.isAllowed(locality, TaskLocality.RACK_LOCAL)) { | ||
| for (rack <- sched.getRackForHost(host)) { | ||
| for (index <- speculatableTasks if canRunOnHost(index)) { |
There was a problem hiding this comment.
nit, but I'd actually refactor this as a single for:
for {
rack <- sched.getRackForHost(host)
index <- speculatableTasks if canRunOnHost(index)
} {
val racks = tasks(index).preferredLocations.map(_.host).flatMap(sched.getRackForHost)
if (racks.contains(rack)) {
speculatableTasks -= index
return Some((index, TaskLocality.RACK_LOCAL))
}
}There was a problem hiding this comment.
not a big deal but I think that can happen outside of this patch since it's just cleanup
There was a problem hiding this comment.
yup, it's style, not substance
|
Test build #51488 has finished for PR 11253 at commit
|
|
LGTM |
…pares Option and String directly. ## What changes were proposed in this pull request? Fix some comparisons between unequal types that cause IJ warnings and in at least one case a likely bug (TaskSetManager) ## How was the this patch tested? Running Jenkins tests Author: Sean Owen <[email protected]> Closes #11253 from srowen/SPARK-13371. (cherry picked from commit 7856253) Signed-off-by: Andrew Or <[email protected]>
|
merged into master 1.6 |
|
Test build #51494 has finished for PR 11253 at commit
|
4 participants
What changes were proposed in this pull request?
Fix some comparisons between unequal types that cause IJ warnings and in at least one case a likely bug (TaskSetManager)
How was the this patch tested?
Running Jenkins tests