You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/12/05 10:21:16 UTC
[GitHub] [flink] xintongsong commented on a diff in pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org