pan3793 commented on code in PR #7626:
URL: https://github.com/apache/kyuubi/pull/7626#discussion_r3765246717
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -58,8 +59,7 @@ case class SparkSQLEngine(spark: SparkSession) extends
Serverable("SparkSQLEngin
@volatile private var lifetimeTerminatingChecker:
Option[ScheduledExecutorService] = None
@volatile private var stopEngineExec: Option[ThreadPoolExecutor] = None
- private lazy val engineSavePath =
-
backendService.sessionManager.asInstanceOf[SparkSQLSessionManager].getEngineResultSavePath()
+ @volatile private var engineSavePath: Path = _
Review Comment:
prefer `Option` over `null`
```suggestion
@volatile private var engineSavePath: Option[Path] = None
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -110,13 +112,15 @@ case class SparkSQLEngine(spark: SparkSession) extends
Serverable("SparkSQLEngin
exec,
Duration(60, TimeUnit.SECONDS))
})
- try {
- val fs =
engineSavePath.getFileSystem(spark.sparkContext.hadoopConfiguration)
- if (fs.exists(engineSavePath)) {
- fs.delete(engineSavePath, true)
+ if (engineSavePath != null) {
+ try {
+ val fs =
engineSavePath.getFileSystem(spark.sparkContext.hadoopConfiguration)
+ if (fs.exists(engineSavePath)) {
+ fs.delete(engineSavePath, true)
+ }
+ } catch {
+ case e: Throwable => error(s"Error cleaning engine result save path:
$engineSavePath", e)
}
- } catch {
- case e: Throwable => error(s"Error cleaning engine result save path:
$engineSavePath", e)
}
Review Comment:
```suggestion
engineSavePath.foreach { path =>
try {
val fs = path.getFileSystem(spark.sparkContext.hadoopConfiguration)
if (fs.exists(path)) {
fs.delete(path, true)
}
} catch {
case e: Throwable => error(s"Error cleaning engine result save path:
$path", e)
}
}
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -90,11 +90,13 @@ case class SparkSQLEngine(spark: SparkSession) extends
Serverable("SparkSQLEngin
startFastFailChecker(maxInitTimeout)
}
- if
(backendService.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE)) {
- val fs =
engineSavePath.getFileSystem(spark.sparkContext.hadoopConfiguration)
- fs.mkdirs(engineSavePath)
- fs.deleteOnExit(engineSavePath)
- }
+ // Due to the fact that session-level enabling of
`kyuubi.operation.result.saveToFile.enabled`
+ // is allowed, we always create and clean up this directory
+ engineSavePath =
+
backendService.sessionManager.asInstanceOf[SparkSQLSessionManager].getEngineResultSavePath()
+ val fs =
engineSavePath.getFileSystem(spark.sparkContext.hadoopConfiguration)
+ fs.mkdirs(engineSavePath)
+ fs.deleteOnExit(engineSavePath)
Review Comment:
```suggestion
val savePath =
backendService.sessionManager.asInstanceOf[SparkSQLSessionManager].getEngineResultSavePath()
engineSavePath = Some(savePath)
val fs = savePath.getFileSystem(spark.sparkContext.hadoopConfiguration)
fs.mkdirs(savePath)
fs.deleteOnExit(savePath)
```
--
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]
