[ZEPPELIN-6462] Close interpreter-setting.json streams with try-with-resources

[big-cir]

What is this PR for?

InterpreterSettingManager discovers interpreters by reading each interpreter's interpreter-setting.json. Two helpers do this, and neither closes the stream it opens:

// registerInterpreterFromResource
getInterpreterListFromJson(url.openStream());

// registerInterpreterFromPath
getInterpreterListFromJson(new FileInputStream(interpreterJsonPath.toFile()));

The shared sink wraps the stream in an InputStreamReader and hands it to gson.fromJson(...). Gson does not close a reader passed to it — the caller owns it — so the descriptor leaks on the normal path. There is no finally or try-with-resources either, so it also leaks when parsing throws, for example a JsonSyntaxException from a malformed setting file.

This is not limited to startup. After installing an interpreter through POST /api/interpreter/install, InterpreterService.downloadInterpreter() calls refreshInterpreterTemplates(), which re-runs the whole directory scan. Every install therefore leaks one descriptor per interpreter directory, and the leaks accumulate on a long-running server.

This PR wraps each stream in a try-with-resources at the call site that opens it. Parsing and registration behaviour is unchanged. No signatures or access modifiers change.

What type of PR is it?

Bug Fix

Todos

• Close the stream opened in registerInterpreterFromResource
• Close the stream opened in registerInterpreterFromPath

What is the Jira issue?

• https://issues.apache.org/jira/browse/ZEPPELIN-6462

How should this be tested?

./mvnw package -pl zeppelin-server --am \
  -Dtest='InterpreterSettingManagerTest,InterpreterFactoryTest,InterpreterSettingTest' \
  -DfailIfNoTests=false

Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, and zeppelin-server builds.

No tests accompany this change. Asserting that a stream is closed requires a seam in production code to inject a tracked stream, and an earlier revision of this PR added one — that is not worth carrying for a two-line fix, so it has been removed along with the tests that used it. The remaining change is the try-with-resources itself.

Screenshots (if appropriate)

N/A

Questions:

• Does the license files need to update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? No

[Reamer]

I think your fix is good, but we shouldn't change the access modifier just for testing purposes. I would also remove the additional methods for testing from the production code. In general, I don't think this change requires any additional tests.

[big-cir]

I think your fix is good, but we shouldn't change the access modifier just for testing purposes. I would also remove the additional methods for testing from the production code. In general, I don't think this change requires any additional tests.

@Reamer
Thanks for the review and the helpful suggestions.

I’ve updated the PR so that access modifiers are no longer changed for testing, and the test-only helper methods have been removed from the production code. The change is now limited to adding try-with-resources.

Please let me know if this looks better.

[Reamer]

Yes, that's how it works. Unnecessary tests are avoided, which reduces the load on the CI.

[big-cir]

Yes, that's how it works. Unnecessary tests are avoided, which reduces the load on the CI.

Thanks for confirming and for the review. I’ll keep this in mind for future PR.

[tbonelee]

Merged into master