XComp commented on code in PR #19275:
URL: https://github.com/apache/flink/pull/19275#discussion_r848497903
##########
flink-runtime/src/test/java/org/apache/flink/runtime/dispatcher/DispatcherTest.java:
##########
@@ -992,6 +992,7 @@ public void
testRequestMultipleJobDetails_returnsFinishedOverSuspendedJob() thro
// run second job, which completes with FINISHED
dispatcherGateway.submitJob(jobGraph, TIMEOUT).get();
+ dispatcherGateway.requestJobResult(jobId, TIMEOUT).get();
Review Comment:
The error that appears here is caused by the `TestJobManagerRunner` not
setting the `getJobDetailsFunction`. I think, fixing this issue in the
`DispatcherTest#completedJobManagerRunnerWithJobStatus` method is the better
way to handle this issue. Please see
https://gist.github.com/XComp/c795bda6a2f157b5a04c8303611b9580
##########
flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/Dispatcher.java:
##########
@@ -1103,23 +1095,60 @@ private void archiveExecutionGraph(ExecutionGraphInfo
executionGraphInfo) {
executionGraphInfo.getArchivedExecutionGraph().getJobID(),
e);
}
+ }
+
+ private CompletableFuture<Acknowledge>
archiveExecutionGraphToHistoryServer(
+ ExecutionGraphInfo executionGraphInfo) {
+
+ return historyServerArchivist
+ .archiveExecutionGraph(executionGraphInfo)
+ .handleAsync(
+ (Acknowledge ignored, Throwable throwable) -> {
+ if (throwable != null) {
+ log.info(
+ "Could not archive completed job
{}({}) to the history server.",
+
executionGraphInfo.getArchivedExecutionGraph().getJobName(),
+
executionGraphInfo.getArchivedExecutionGraph().getJobID(),
+ throwable);
+ }
+ return Acknowledge.get();
+ },
+ getMainThreadExecutor());
Review Comment:
Thanks for sharing you're concern, @Thesharing . It's valid to think about
it. But @zentol: it shouldn't matter where `CompletableFuture` was executed,
right? ...considering that in `ApplicationDispatcherBootstrap`, we only
complete the `jobIdsFuture` which can also run in the ioExecutor (because the
`CompletableFuture#complete` method is thread-safe). Do you agree?
In general, I would say, that, if a async method doesn't give any indication
on where it is executed (through the method signatute, name or at least
JavaDoc), the caller should not assume things. Therefore, we should make it
explicit in the calling context of `ApplicationDispatcherBootstrap`, if it
would be a problem to call the `thenAccept` code block not within the main
thread by using `thenAcceptAsync` there.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
