JoshRosen commented on code in PR #36775:
URL: https://github.com/apache/spark/pull/36775#discussion_r925281565
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileScanRDD.scala:
##########
@@ -215,6 +215,17 @@ class FileScanRDD(
}
}
+ private def isDFSClientClosedException(e: IOException): Boolean = {
+ e.getStackTrace.foreach { ste: StackTraceElement =>
+ if (ste != null && ste.getMethodName != null && ste.getClassName !=
null
+ && ste.getClassName.contains("DFSClient")
+ && ste.getMethodName.contains("checkOpen")) {
+ true
+ }
+ }
+ false
+ }
Review Comment:
I don't think this use of `foreach` is correct: `foreach` returns Unit`, so
this `isDFSClientClosedException` always returns `false` via the statement
below the `foreach`. Instead, I think you should use `exists`:
```suggestion
private def isDFSClientClosedException(e: IOException): Boolean = {
e.getStackTrace.exists { ste: StackTraceElement =>
(ste != null
&& ste.getMethodName != null
&& ste.getClassName != null
&& ste.getClassName.contains("DFSClient")
&& ste.getMethodName == "checkOpen")
}
}
```
Here's a test of this in a scala REPL:
```scala
scala> import java.io._
import java.io._
scala> import java.lang._
import java.lang._
scala> class DFSClient { def checkOpen(): Unit = { throw new
IOException("e") } }
defined class DFSClient
scala> def isDFSClientClosedException(e: IOException): Boolean = {
| e.getStackTrace.exists { ste: StackTraceElement =>
| (ste != null
| && ste.getMethodName != null
| && ste.getClassName != null
| && ste.getClassName.contains("DFSClient")
| && ste.getMethodName == "checkOpen")
| }
| }
isDFSClientClosedException: (e: java.io.IOException)Boolean
scala> isDFSClientClosedException(scala.util.Try((new
DFSClient).checkOpen()).failed.get.asInstanceOf[IOException])
res0: Boolean = true
scala> isDFSClientClosedException(new IOException("e"))
res1: Boolean = false
```
In this new code, I've kept `.contains` for the class name check but
switched to `==` for checking the method name. I think it's a good idea to be
more permissive for class names so that we can handle shading, but I don't
expect that method names would be renamed so I think it's fine to keep the
method name as an equality check.
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
