sqd commented on code in PR #29136:
URL: https://github.com/apache/flink/pull/29136#discussion_r3969405532
##########
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:
Good idea. Will do
##########
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:
Hi, thanks for taking a look! I think "one job missing **silently**" is
dangerous not only for the operator, but also for an engineer who is using the
UI to determine what to do. The engineer may draw wrong conclusion and actions
from the UI. Failing loud as a 500 for now is an improvement in my opinion.
I agree with you that UX-wise this is a regression. I think something we can
do in a followup PR, is to either cache previous good listing in the frontend,
and show a banner along the line of "cannot contact jobmanager to update job
list, what's currently shown may be stale", or render an empty job list with a
similar message.
--
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]
