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/11/29 16:32:32 UTC
[GitHub] [flink] 1996fanrui opened a new pull request, #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
1996fanrui opened a new pull request, #21420:
URL: https://github.com/apache/flink/pull/21420
## What is the purpose of the change
Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
## Brief change log
- The first commit : LOG the exception while generating the flame graph and refactor some code (It's useful to trouble-shooting int the future)
- The second commit: Remove the ImmutableSet
## Verifying this change
This change added tests and can be verified as follows:
- *JobVertexThreadInfoTrackerTest#testGroupExecutionsByLocation*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not documented
--
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
[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
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [flink] 1996fanrui commented on pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #21420:
URL: https://github.com/apache/flink/pull/21420#issuecomment-1338987248
@xintongsong , thanks for your review!
--
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
[GitHub] [flink] 1996fanrui commented on a diff in pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #21420:
URL: https://github.com/apache/flink/pull/21420#discussion_r1039719559
##########
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:
Thanks for your suggestion.
I revoked this change, and check the `ImmutableSet<ExecutionAttemptID>` in `TestingThreadInfoRequestCoordinator#triggerThreadInfoRequest`.
It can cover this bug and check the `ImmutableSet<ExecutionAttemptID>` during request thread info for all unit tests.
--
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
[GitHub] [flink] 1996fanrui commented on pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #21420:
URL: https://github.com/apache/flink/pull/21420#issuecomment-1330940003
Hi @afedulov @fapaul , this is a bug about flame graph and it's caused by #19228 [FLINK-26074].
Please help take a look in your free time, thanks~
--
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
[GitHub] [flink] xintongsong closed pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
xintongsong closed pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
URL: https://github.com/apache/flink/pull/21420
--
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
[GitHub] [flink] flinkbot commented on pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21420:
URL: https://github.com/apache/flink/pull/21420#issuecomment-1330936271
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7c13a7f3a672c6f53d682badd2b9b54c83360f8d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "7c13a7f3a672c6f53d682badd2b9b54c83360f8d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7c13a7f3a672c6f53d682badd2b9b54c83360f8d UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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
[GitHub] [flink] 1996fanrui commented on pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #21420:
URL: https://github.com/apache/flink/pull/21420#issuecomment-1334741798
Hi @xintongsong , this is a bug about flame graph and it's caused by #19228 [FLINK-26074].
The flame graph doesn't work due this bug after flink 1.16, please help take a look in your free time, thanks~
--
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
[GitHub] [flink] 1996fanrui commented on a diff in pull request #21420: [FLINK-30239][rest] Fix the problem that FlameGraph cannot be generated due to using ImmutableSet incorrectly
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #21420:
URL: https://github.com/apache/flink/pull/21420#discussion_r1039712703
##########
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:
Hi @xintongsong , thanks for your review.
Sorry, I missed that log before due to that log level is debug.
I changed the log level to error when flame graph generation fails, it's useful to trouble-shooting in the future.
<img width="972" alt="image" src="https://user-images.githubusercontent.com/38427477/205669346-c28e6bb5-589a-420e-a1c2-43b7ef2ce146.png">
--
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