renechoi commented on code in PR #5000:
URL: https://github.com/apache/zeppelin/pull/5000#discussion_r2250059133
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/SparkInterpreterLauncher.java:
##########
@@ -272,22 +272,34 @@ private String detectSparkScalaVersion(String sparkHome,
Map<String, String> env
builder.environment().putAll(env);
File processOutputFile = File.createTempFile("zeppelin-spark", ".out");
builder.redirectError(processOutputFile);
- Process process = builder.start();
- process.waitFor();
- String processOutput = IOUtils.toString(new
FileInputStream(processOutputFile), StandardCharsets.UTF_8);
- Pattern pattern = Pattern.compile(".*Using Scala version (.*),.*");
- Matcher matcher = pattern.matcher(processOutput);
- if (matcher.find()) {
- String scalaVersion = matcher.group(1);
- if (scalaVersion.startsWith("2.12")) {
- return "2.12";
- } else if (scalaVersion.startsWith("2.13")) {
- return "2.13";
+
+ try {
+ Process process = builder.start();
+ process.waitFor();
+
+ String processOutput;
+ try (FileInputStream in = new FileInputStream(processOutputFile)) {
+ processOutput = IOUtils.toString(in, StandardCharsets.UTF_8);
+ }
+
+ Pattern pattern = Pattern.compile(".*Using Scala version (.*),.*");
+ Matcher matcher = pattern.matcher(processOutput);
+ if (matcher.find()) {
+ String scalaVersion = matcher.group(1);
+ if (scalaVersion.startsWith("2.12")) {
+ return "2.12";
+ } else if (scalaVersion.startsWith("2.13")) {
+ return "2.13";
+ } else {
+ throw new Exception("Unsupported scala version: " + scalaVersion);
+ }
} else {
- throw new Exception("Unsupported scala version: " + scalaVersion);
+ return detectSparkScalaVersionByReplClass(sparkHome);
+ }
+ } finally {
+ if (!processOutputFile.delete() && processOutputFile.exists()) {
Review Comment:
@Reamer Thank you for the review! I've updated the implementation based on
your suggestions:
Changes made:
- Removed the need for temporary files entirely by capturing the process
error stream directly using `IOUtils.toString(process.getErrorStream(),
StandardCharsets.UTF_8)`
- This eliminates the need for `File.createTempFile`, `FileInputStream`,
and `Files.deleteIfExists`
- Updated the unit tests to reflect the new implementation without temp
file handling
The solution is now simpler, more efficient, and completely eliminates the
resource leak risk by not using temporary files at all.
--
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...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
