spuru9 commented on code in PR #29136:
URL: https://github.com/apache/flink/pull/29136#discussion_r3964699135
##########
flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/Dispatcher.java:
##########
@@ -1627,15 +1627,27 @@ private JobsOverview getCompletedJobsOverview() {
@Override
public CompletableFuture<MultipleJobsDetails>
requestMultipleJobDetails(Duration timeout) {
- List<CompletableFuture<Optional<JobDetails>>>
individualOptionalJobDetails =
- queryJobMastersForInformation(
- jobManagerRunner ->
jobManagerRunner.requestJobDetails(timeout));
-
- CompletableFuture<Collection<Optional<JobDetails>>>
optionalCombinedJobDetails =
- FutureUtils.combineAll(individualOptionalJobDetails);
+ // A job whose JobMaster cannot answer must fail the whole request
rather than be
+ // silently left out: clients treat absence from this list as the job
being gone.
+ final List<CompletableFuture<JobDetails>> individualJobDetails =
+ new ArrayList<>(jobManagerRunnerRegistry.size());
+ for (JobManagerRunner jobManagerRunner :
jobManagerRunnerRegistry.getJobManagerRunners()) {
Review Comment:
Question: This is the endpoint the Web UI job list polls too, not just the
K8s operator. With this change, one running job whose JobMaster is briefly slow
or times out on requestJobDetails now takes down the entire overview (500)
instead of returning the other healthy jobs. For the operator, failing loudly
is clearly the right call; for the UI it's a regression from "one job missing"
to "whole page errors." Did you weigh that trade-off?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/dispatcher/DispatcherTest.java:
##########
@@ -1407,6 +1407,54 @@ public void
testRequestMultipleJobDetails_returnsJobsOfSameStateOrderedByStartTi
Stream.of(jobId,
secondJobID).sorted().collect(Collectors.toList()));
}
+ /**
+ * A JobMaster that fails or times out on {@code requestJobDetails} must
not cause its running
+ * job to be silently omitted from an otherwise successful response:
clients (such as the
+ * Kubernetes operator) treat absence from this list as "job not found".
+ */
+ @Test
+ public void
testRequestMultipleJobDetails_doesNotSilentlyOmitJobWhoseJobMasterQueryFails()
+ throws Exception {
+ final JobID secondJobID = new JobID();
+ JobGraph secondJobGraph = JobGraphTestUtils.streamingJobGraph();
+ secondJobGraph.setJobID(secondJobID);
+ secondJobGraph.setApplicationId(applicationId);
+ final JobManagerRunner unresponsiveJobManagerRunner =
+ TestingJobManagerRunner.newBuilder()
+ .setJobId(secondJobID)
+ .setJobDetailsFutureFunction(
+ () ->
+ FutureUtils.completedExceptionally(
+ new TimeoutException(
+ "JobMaster did not
answer in time")))
+ .build();
+ final JobManagerRunnerFactory jobManagerRunnerFactory =
+ new QueuedJobManagerRunnerFactory(
+
runningJobManagerRunnerWithJobStatus(JobStatus.RUNNING, jobId, 10L),
+ unresponsiveJobManagerRunner);
+
+ DispatcherGateway dispatcherGateway =
+ createDispatcherAndStartJobs(
+ jobManagerRunnerFactory, Arrays.asList(jobGraph,
secondJobGraph));
+
+ final CompletableFuture<MultipleJobsDetails> multipleJobsDetailsFuture
=
+ dispatcherGateway.requestMultipleJobDetails(TIMEOUT);
+
+ final MultipleJobsDetails multipleJobsDetails;
+ try {
+ multipleJobsDetails = multipleJobsDetailsFuture.get();
+ } catch (ExecutionException e) {
+ // Failing the whole request is acceptable: the client learns that
the view is
+ // incomplete instead of concluding that the job is gone.
+ return;
Review Comment:
We can add a assertion to the fail to be sure no other exception slip
through. WDYT?
```
} catch (ExecutionException e) {
// Failing the whole request is acceptable, but it must fail for
the right reason:
// an exception naming the job whose JobMaster could not be
queried.
assertThat(e)
.hasStackTraceContaining("Could not retrieve the details
of job")
.hasStackTraceContaining(secondJobID.toString());
return;
}
```
--
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]
