[FLINK-14816] Add thread dump feature for taskmanager #11887

tillrohrmann · 2020-04-23T16:22:36Z

Extension of #10228.

flinkbot · 2020-04-23T16:26:27Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 04664a7 (Thu Apr 23 16:26:27 UTC 2020)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-04-23T16:37:44Z

CI report:

89f3c0d Travis: SUCCESS Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

lamberken · 2020-04-23T19:41:27Z

hi @tillrohrmann, thanks for doing this, 💯 , but RestAPIStabilityTest run failed.

Worked fine.

tillrohrmann · 2020-04-24T08:11:55Z

Thanks for the pointer @lamber-ken. I will update the PR to fix the RestAPIStabilityTest.

Instead of serving plain text, this commit changes the TaskManagerThreadDumpHandler to return a JSON response. This allows to further extend this handler to not only generate a thread dump of all threads but also for a sub set. Morever, it allows to return a more structured return value in the future. This closes apache#11887.

tillrohrmann · 2020-04-24T13:07:03Z

cc @zentol: The TaskManagerThreadDumpHandler now returns a ThreadDumpInfo which contains an array of ThreadDumpInfo.ThreadInfo. ThreadDumpInfo.ThreadInfo contains the name of the thread and the stringified thread info. This is kind of the compromise I found wrt our offline discussion.

Instead of serving plain text, this commit changes the TaskManagerThreadDumpHandler to return a JSON response. This allows to further extend this handler to not only generate a thread dump of all threads but also for a sub set. Morever, it allows to return a more structured return value in the future. This closes apache#11887.

lamberken · 2020-04-26T02:50:11Z

flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java

+	private CompletableFuture<TransientBlobKey> putTransientBlobStream(InputStream inputStream, String fileTag) {
+		final TransientBlobCache transientBlobService = blobCacheService.getTransientBlobService();
+		final TransientBlobKey transientBlobKey;
+
+		try {
+			transientBlobKey = transientBlobService.putTransient(inputStream);
+		} catch (IOException e) {
+			log.debug("Could not upload file {}.", fileTag, e);
+			return FutureUtils.completedExceptionally(new FlinkException("Could not upload file " + fileTag + '.', e));
+		}
+		return CompletableFuture.completedFuture(transientBlobKey);
+	}
+


hi @tillrohrmann, since doesn't make thread dump as stream in your design right now, we can revert it.

I think this change is still good since it breaks up a longer method into smaller units.

They still belong into a separate commit though.

I think this change is still good since it breaks up a longer method into smaller units.

ok

Technically yes but I think it is ok to not twiddle with this commit because the benefit would be marginal.

zentol · 2020-04-27T11:31:14Z

flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/FileType.java

@@ -22,6 +22,13 @@
 * Different file types to request from the {@link TaskExecutor}.
 */
 public enum FileType {
+	/**
+	 * the log file type for taskmanager


this seems unrelated?

Somewhat. But I believe that the extra effort splitting this change into a separate commit won't pay off. Hence, I would like to keep it as is.

Per the code-style guide:

Pull Requests must put cleanup, refactoring, and core changes into separate commits.

You are right Chesnay. I will refactor the changes into separate commits in order to follow the code style guide.

zentol · 2020-04-27T11:36:11Z

flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java

+		final Collection<ThreadInfo> threadDump = JvmUtils.createThreadDump();
+
+		final Collection<ThreadDumpInfo.ThreadInfo> threadInfos = threadDump.stream()
+			.map(threadInfo -> ThreadDumpInfo.ThreadInfo.create(threadInfo.getThreadName(), threadInfo.toString()))


I'm wondering whether this conversion should really be done on the TaskExecutor.
I'd prefer if conversions to REST responses are done by the handler just to separate things a bit.

As soon as the Rest response and the internal cluster message differ I will separate the classes. As long as they are identical I think it is fine to use the same class here.

zentol · 2020-04-27T11:40:15Z

...runtime/src/main/java/org/apache/flink/runtime/rest/messages/taskmanager/ThreadDumpInfo.java

+	public static final String FIELD_NAME_THREAD_INFOS = "threadInfos";
+
+	@JsonProperty(FIELD_NAME_THREAD_INFOS)
+	private final Collection<ThreadInfo> threadInfos;


Why not a map thread_name -> thread_info? I think this would be more generally usable.

The main reason was the duplicated thread name information as a key and as part of the ThreadInfo class. But I guess this is premature optimization. I will change it.

Actually, I think whether it is stored in a map or not is an implementation detail. Moreover, we don't offer a method getThreadInfo(String threadName) yet. Hence, there is actually no reason to store them internally as a map. Once the requirement changes, I'm happy to adapt the class.

I wasn't talking about changing the internal storage, but to expose it as a map instead (i.e., changing the JOSN representation)

Assuming a user is interested in a particular thread, then with a map one can do threadDump['myThreadName'], instead of having to iterate over the array.

Good point. I will take another look.

After having looked at it again, I still believe that the response should be a collection of ThreadInfos. If the user needs to do a named lookup, then she can still create a map from the collection. It should not be the responsibility response class to present the elements in a certain order. Moreover, we don't have this requirement at the moment. Hence, I won't do the proposed change.

zentol · 2020-04-27T11:43:17Z

flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java

+	private CompletableFuture<TransientBlobKey> putTransientBlobStream(InputStream inputStream, String fileTag) {
+		final TransientBlobCache transientBlobService = blobCacheService.getTransientBlobService();
+		final TransientBlobKey transientBlobKey;
+
+		try {
+			transientBlobKey = transientBlobService.putTransient(inputStream);
+		} catch (IOException e) {
+			log.debug("Could not upload file {}.", fileTag, e);
+			return FutureUtils.completedExceptionally(new FlinkException("Could not upload file " + fileTag + '.', e));
+		}
+		return CompletableFuture.completedFuture(transientBlobKey);
+	}
+


They still belong into a separate commit though.

tillrohrmann · 2020-04-27T13:58:20Z

Thanks for the review @zentol. I answered all your comments. Please take another look.

Instead of serving plain text, this commit changes the TaskManagerThreadDumpHandler to return a JSON response. This allows to further extend this handler to not only generate a thread dump of all threads but also for a sub set. Morever, it allows to return a more structured return value in the future. This closes apache#11887.

…Path

Instead of serving plain text, this commit changes the TaskManagerThreadDumpHandler to return a JSON response. This allows to further extend this handler to not only generate a thread dump of all threads but also for a sub set. Morever, it allows to return a more structured return value in the future. This closes apache#11887.

…ponse

tillrohrmann · 2020-04-28T11:55:33Z

Thanks for the review @lamber-ken.

@zentol I've split the unrelated changes into separate commits. I won't change the JSON format though. I will merge this PR once CI gives green light.

tillrohrmann mentioned this pull request Apr 23, 2020

[FLINK-14816] Add thread dump feature for taskmanager #10228

Closed

rmetzger added the review=description? label Apr 23, 2020

rmetzger added the component=Runtime/WebFrontend label Apr 23, 2020

tillrohrmann force-pushed the FLINK-14816 branch from 04664a7 to 9c2c469 Compare April 24, 2020 08:19

tillrohrmann marked this pull request as ready for review April 24, 2020 08:19

tillrohrmann changed the title ~~[DRAFT][FLINK-14816] Add thread dump feature for taskmanager~~ [FLINK-14816] Add thread dump feature for taskmanager Apr 24, 2020

tillrohrmann force-pushed the FLINK-14816 branch from 9c2c469 to c81293a Compare April 24, 2020 08:23

tillrohrmann force-pushed the FLINK-14816 branch from c81293a to b84b3cb Compare April 24, 2020 13:02

tillrohrmann force-pushed the FLINK-14816 branch from 852c752 to bc5fd41 Compare April 24, 2020 13:12

lamberken reviewed Apr 26, 2020

View reviewed changes

zentol self-assigned this Apr 27, 2020

zentol reviewed Apr 27, 2020

View reviewed changes

tillrohrmann force-pushed the FLINK-14816 branch from bc5fd41 to 5ecc88f Compare April 27, 2020 13:59

lamberken approved these changes Apr 28, 2020

View reviewed changes

lamberken and others added 5 commits April 28, 2020 12:10

[hotfix] Add comments to FileType enum values

b32085c

[hotfix] Factor out putTransientBlobStream from requestFileuploadFile…

27088c6

…Path

[FLINK-14816] Add thread dump feature for taskmanager

3e650a5

fixup! [FLINK-14816] Let TaskManagerThreadDumpHandler return JSON res…

89f3c0d

…ponse

tillrohrmann force-pushed the FLINK-14816 branch from 5ecc88f to 89f3c0d Compare April 28, 2020 11:54

tillrohrmann closed this in 4dc1f7e May 1, 2020

tillrohrmann deleted the FLINK-14816 branch May 1, 2020 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-14816] Add thread dump feature for taskmanager #11887

[FLINK-14816] Add thread dump feature for taskmanager #11887

tillrohrmann commented Apr 23, 2020

flinkbot commented Apr 23, 2020

flinkbot commented Apr 23, 2020 •

edited

Loading

lamberken commented Apr 23, 2020 •

edited

Loading

tillrohrmann commented Apr 24, 2020

tillrohrmann commented Apr 24, 2020

lamberken Apr 26, 2020

tillrohrmann Apr 27, 2020

zentol Apr 27, 2020

lamberken Apr 27, 2020

tillrohrmann Apr 27, 2020

zentol Apr 27, 2020

tillrohrmann Apr 27, 2020

zentol Apr 27, 2020

tillrohrmann Apr 28, 2020

zentol Apr 27, 2020

tillrohrmann Apr 27, 2020 •

edited

Loading

zentol Apr 27, 2020

tillrohrmann Apr 27, 2020

tillrohrmann Apr 27, 2020

zentol Apr 27, 2020

tillrohrmann Apr 28, 2020

tillrohrmann Apr 28, 2020

zentol Apr 27, 2020

tillrohrmann commented Apr 27, 2020

tillrohrmann commented Apr 28, 2020

[FLINK-14816] Add thread dump feature for taskmanager #11887

[FLINK-14816] Add thread dump feature for taskmanager #11887

Conversation

tillrohrmann commented Apr 23, 2020

flinkbot commented Apr 23, 2020

Automated Checks

Review Progress

flinkbot commented Apr 23, 2020 • edited Loading

CI report:

lamberken commented Apr 23, 2020 • edited Loading

tillrohrmann commented Apr 24, 2020

tillrohrmann commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann commented Apr 27, 2020

tillrohrmann commented Apr 28, 2020

flinkbot commented Apr 23, 2020 •

edited

Loading

lamberken commented Apr 23, 2020 •

edited

Loading

tillrohrmann Apr 27, 2020 •

edited

Loading