LuciferYang commented on code in PR #13107:
URL: https://github.com/apache/gluten/pull/13107#discussion_r4082091594
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -290,12 +294,38 @@ class TaskResourceRegistry extends Logging {
/** Release all managed resources according to priority and reversed order */
private[task] def releaseAll(): Unit = lock {
+ val failures = mutable.ArrayBuffer.empty[Throwable]
priorityToResourcesMapping.toSeq.sortBy(-_._1).foreach {
case (_, resources) =>
- resources.toSeq.reverse.foreach(release)
+ resources.toSeq.reverse.foreach {
+ resource =>
+ // resourceName() is user code too; parse it with a fallback so it
+ // cannot re-abort the release loop.
+ val name =
+ try resource.resourceName()
+ catch {
+ case NonFatal(_) =>
s"resource@${System.identityHashCode(resource)}"
Review Comment:
Moved the `resourceName()` lookup into the failure branch and widened its
catch to `Throwable` to match the release path, so an `Error` while building
the log label can no longer abort the loop. Also removed the now-unused
`NonFatal` import. Fixed in 12b0458a4.
##########
gluten-core/src/main/scala/org/apache/spark/task/TaskResources.scala:
##########
@@ -290,12 +294,38 @@ class TaskResourceRegistry extends Logging {
/** Release all managed resources according to priority and reversed order */
private[task] def releaseAll(): Unit = lock {
+ val failures = mutable.ArrayBuffer.empty[Throwable]
priorityToResourcesMapping.toSeq.sortBy(-_._1).foreach {
case (_, resources) =>
- resources.toSeq.reverse.foreach(release)
+ resources.toSeq.reverse.foreach {
+ resource =>
+ // resourceName() is user code too; parse it with a fallback so it
+ // cannot re-abort the release loop.
+ val name =
+ try resource.resourceName()
+ catch {
+ case NonFatal(_) =>
s"resource@${System.identityHashCode(resource)}"
+ }
+ try release(resource)
+ catch {
+ case e: Throwable =>
+ // One failing release must not skip the remaining ones or
leave the
+ // registry uncleared; record the failure and rethrow it after
the
+ // loop so callers still see the error.
+ failures += e
+ logError(s"Failed to release resource $name", e)
+ }
+ }
}
priorityToResourcesMapping.clear()
resources.clear()
+ failures.headOption.foreach {
+ failure =>
+ // Keep the remaining failures attached; the logs are the only other
+ // record and may be swallowed by the completion-listener machinery.
+ failures.tail.foreach(failure.addSuppressed)
Review Comment:
Now filtering out entries identical to `failure` by reference
(`failures.tail.filterNot(_ eq failure)`) before `addSuppressed`, so a repeated
head instance no longer triggers the self-suppression
`IllegalArgumentException` and the original failure is always rethrown. Fixed
in 12b0458a4.
##########
gluten-core/src/test/scala/org/apache/gluten/task/TaskResourceSuite.scala:
##########
@@ -83,4 +83,33 @@ class TaskResourceSuite extends AnyFunSuite with SQLHelper {
}
assert(unregisteredCount == 2)
}
+
+ test("Run unsafe - one failing release does not skip the remaining
resources") {
+ var goodReleased = 0
+ // Whether the rethrown release failure surfaces from runUnsafe depends on
+ // the Spark version's completion-listener handling, so tolerate both
+ // outcomes and pin only the invariant: the good resource is released
+ // despite the failure.
+ scala.util.Try(TaskResources.runUnsafe {
Review Comment:
The test now asserts the release failure is rethrown (`result.isFailure`,
and the printed stack trace contains "release failed") and that a fresh task
runs cleanly afterwards, instead of discarding the `Try`. Fixed in 12b0458a4.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
