MartijnVisser commented on code in PR #29473:
URL: https://github.com/apache/flink/pull/29473#discussion_r4427358012
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/utils/JarHandlerUtils.java:
##########
@@ -162,8 +167,15 @@ public void applyToConfiguration(
PipelineOptionsInternal.PIPELINE_FIXED_JOB_ID,
jobId.toHexString());
}
configuration.set(CoreOptions.DEFAULT_PARALLELISM, parallelism);
+ }
- final PackagedProgram program = toPackagedProgram(configuration);
+ /**
+ * Applies the jars and classpaths of the given program to the
configuration. The jars may
+ * include nested libraries extracted by the program, which are only
valid until the program
+ * is closed.
+ */
+ public void applyProgramToConfiguration(
Review Comment:
Could `toPackagedProgram` set `pipeline.jars` and `pipeline.classpaths`
itself? Then the handlers stay unchanged and can't miss the second call.
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/JarRunHandler.java:
##########
@@ -103,6 +103,12 @@ public CompletableFuture<JarRunResponseBody> handleRequest(
effectiveConfiguration);
final PackagedProgram program =
context.toPackagedProgram(effectiveConfiguration);
+ try {
+ context.applyProgramToConfiguration(effectiveConfiguration,
program);
+ } catch (RuntimeException e) {
Review Comment:
I don't think this can throw, since the constructor already called
`getJobJarAndDependencies`. I would drop this try/catch and the one in
`JarRunApplicationHandler`.
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/handlers/utils/JarHandlerUtilsTest.java:
##########
@@ -123,6 +124,25 @@ void testFromRequestWithParallelismConfig() throws
Exception {
assertThat(jarHandlerContext.getParallelism()).isEqualTo(parallelism);
}
+ @Test
+ void testApplyingRequestConfigurationDoesNotOpenJar() throws Exception {
Review Comment:
This tests the split, not the leak. A test that plans and runs a jar with a
nested `lib/` jar and then checks `java.io.tmpdir` fails on `master`. Please
add that.
--
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]
