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/13 06:09:40 UTC
[GitHub] [flink] yuchen-ecnu opened a new pull request, #21498: [FLINK-30392] Replace default thread dump depth to 24
yuchen-ecnu opened a new pull request, #21498:
URL: https://github.com/apache/flink/pull/21498
## What is the purpose of the change
Currently, the thread dump function still have the default stack-trace depth as 8, which is the same as before. However, the default value is really too small for developers to know the actual thread info.
From our experiences, we can set this value as 24.
## Brief change log
- Replace default value of `cluster.thread-dump.stacktrace-max-depth` to `24`.
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## 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 applicable
--
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] yuchen-ecnu commented on pull request #21498: [FLINK-30392] Replace default thread dump depth to 50
Posted by GitBox <gi...@apache.org>.
yuchen-ecnu commented on PR #21498:
URL: https://github.com/apache/flink/pull/21498#issuecomment-1367782568
Hi @Myasuka @1996fanrui ,
Thanks for the discussion.
As discussed above, I've set the default depth value of the thread dump to 50 and updated the related commit information.
Are there any other comments for this PR?
--
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] Myasuka closed pull request #21498: [FLINK-30392] Replace default thread dump depth to 50
Posted by GitBox <gi...@apache.org>.
Myasuka closed pull request #21498: [FLINK-30392] Replace default thread dump depth to 50
URL: https://github.com/apache/flink/pull/21498
--
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] Myasuka commented on a diff in pull request #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #21498:
URL: https://github.com/apache/flink/pull/21498#discussion_r1050406399
##########
docs/layouts/shortcodes/generated/cluster_configuration.html:
##########
@@ -70,7 +70,7 @@
</tr>
<tr>
<td><h5>cluster.thread-dump.stacktrace-max-depth</h5></td>
- <td style="word-wrap: break-word;">8</td>
+ <td style="word-wrap: break-word;">24</td>
Review Comment:
Actually, we can set this value as 10000 or even larger to make sure we can see the whole stack in any case. However, this might also increase the whole result size of the returned API. The current suggestion of 24 is just based on our experiences in the production environment. I think it's fine to increase the default value to a larger one with your input.
@yuchen-ecnu WDYT?
--
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] yuchen-ecnu commented on a diff in pull request #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
yuchen-ecnu commented on code in PR #21498:
URL: https://github.com/apache/flink/pull/21498#discussion_r1059278748
##########
docs/layouts/shortcodes/generated/cluster_configuration.html:
##########
@@ -70,7 +70,7 @@
</tr>
<tr>
<td><h5>cluster.thread-dump.stacktrace-max-depth</h5></td>
- <td style="word-wrap: break-word;">8</td>
+ <td style="word-wrap: break-word;">24</td>
Review Comment:
Hi @1996fanrui @Myasuka,
With the two instances shared by @1996fanrui , I think it is necessary to further increase the default depth of Thread Dump. How about increasing this value to 50?
--
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 #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #21498:
URL: https://github.com/apache/flink/pull/21498#discussion_r1048482228
##########
docs/layouts/shortcodes/generated/cluster_configuration.html:
##########
@@ -70,7 +70,7 @@
</tr>
<tr>
<td><h5>cluster.thread-dump.stacktrace-max-depth</h5></td>
- <td style="word-wrap: break-word;">8</td>
+ <td style="word-wrap: break-word;">24</td>
Review Comment:
Hi @Myasuka and @yuchen-ecnu , thanks for your contribution. It's a great proposal, the 8 is too small for troubleshooting.
However, I'm afraid the 24 is also too small, could you add some information about why you set it to 24? I may have missed some information.
we set it to 80 in my company. Because some stacks are too deep, like this:
--
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] yuchen-ecnu commented on a diff in pull request #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
yuchen-ecnu commented on code in PR #21498:
URL: https://github.com/apache/flink/pull/21498#discussion_r1051818487
##########
docs/layouts/shortcodes/generated/cluster_configuration.html:
##########
@@ -70,7 +70,7 @@
</tr>
<tr>
<td><h5>cluster.thread-dump.stacktrace-max-depth</h5></td>
- <td style="word-wrap: break-word;">8</td>
+ <td style="word-wrap: break-word;">24</td>
Review Comment:
Hi @1996fanrui , @Myasuka.
I think such a deep call stack might not be required for most scenarios, and in our experience so far 24 seems to be a value that can cover most cases.
In my opinion, to balance the performance of the Rest API and analysis experience, we can consider setting up a deeper call stack with the parameters provided by Flink for such a job.
https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/deployment/config/#cluster-thread-dump-stacktrace-max-depth
--
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 #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21498:
URL: https://github.com/apache/flink/pull/21498#issuecomment-1347801647
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "42ea3e7ac469f3f133cb3672953e675286193a9d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "42ea3e7ac469f3f133cb3672953e675286193a9d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 42ea3e7ac469f3f133cb3672953e675286193a9d 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 a diff in pull request #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #21498:
URL: https://github.com/apache/flink/pull/21498#discussion_r1053070903
##########
docs/layouts/shortcodes/generated/cluster_configuration.html:
##########
@@ -70,7 +70,7 @@
</tr>
<tr>
<td><h5>cluster.thread-dump.stacktrace-max-depth</h5></td>
- <td style="word-wrap: break-word;">8</td>
+ <td style="word-wrap: break-word;">24</td>
Review Comment:
Hi @Myasuka @yuchen-ecnu , thanks for your feedback.
I helped our users troubleshoot some problems this week and found that the stack depth of flink Task threads can easily exceed 24. As shown in the figure: there are only three lines of business code, and nearly 40 lines of flink code.
If a Task contains many operators, the stack depth will be deeper. How about 40 or 50?
--
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] Myasuka commented on a diff in pull request #21498: [FLINK-30392] Replace default thread dump depth to 24
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #21498:
URL: https://github.com/apache/flink/pull/21498#discussion_r1059279745
##########
docs/layouts/shortcodes/generated/cluster_configuration.html:
##########
@@ -70,7 +70,7 @@
</tr>
<tr>
<td><h5>cluster.thread-dump.stacktrace-max-depth</h5></td>
- <td style="word-wrap: break-word;">8</td>
+ <td style="word-wrap: break-word;">24</td>
Review Comment:
@yuchen-ecnu I think it's okay, let's change the default value and the related commit messages.
--
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