xintongsong commented on code in PR #21420:
URL: https://github.com/apache/flink/pull/21420#discussion_r1039404786
##########
flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/threadinfo/JobVertexThreadInfoTracker.java:
##########
@@ -251,7 +251,7 @@ private void triggerThreadInfoSampleInternal(
return executionsWithGateways;
}
- private Map<TaskManagerLocation, ImmutableSet<ExecutionAttemptID>>
groupExecutionsByLocation(
+ Map<TaskManagerLocation, ImmutableSet<ExecutionAttemptID>>
groupExecutionsByLocation(
Review Comment:
Having to change the method visibility for testing is usually an indicator
of testing against internal implementation. We should treat the testee as
blackbox and test against its contract. In this case, the contract should be
that `getVertexStats` should work properly when there's multiple executions for
the same execution vertex.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/threadinfo/JobVertexThreadInfoTracker.java:
##########
@@ -251,7 +251,7 @@ private void triggerThreadInfoSampleInternal(
return executionsWithGateways;
}
- private Map<TaskManagerLocation, ImmutableSet<ExecutionAttemptID>>
groupExecutionsByLocation(
+ Map<TaskManagerLocation, ImmutableSet<ExecutionAttemptID>>
groupExecutionsByLocation(
Review Comment:
And in cases where there's no good way for testing without accessing
internal methods, we should add an annotation `@VisibleForTesting`.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/threadinfo/JobVertexThreadInfoTracker.java:
##########
@@ -203,7 +203,14 @@ private void triggerThreadInfoSampleInternal(
delayBetweenSamples,
maxThreadInfoDepth));
- sample.whenCompleteAsync(new
ThreadInfoSampleCompletionCallback(key, vertex), executor);
+ sample.whenCompleteAsync(new
ThreadInfoSampleCompletionCallback(key, vertex), executor)
+ .exceptionally(
+ throwable -> {
+ LOG.warn(
+ "An exception occurred while
generating the flame graph.",
+ throwable);
+ return null;
+ });
Review Comment:
Isn't `ThreadInfoSampleCompletionCallback` already printing the error?
--
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]
