[GitHub] [tez] abstractdog commented on a diff in pull request #299: TEZ-4344: Collect jstack periodically from all containers.

["abstractdog (via GitHub)" <gi...@apache.org>]

abstractdog commented on code in PR #299:
URL: https://github.com/apache/tez/pull/299#discussion_r1250805095


##########
tez-dag/src/main/java/org/apache/tez/dag/app/DAGAppMaster.java:
##########
@@ -345,7 +352,7 @@ public DAGAppMaster(ApplicationAttemptId applicationAttemptId,
       ContainerId containerId, String nmHost, int nmPort, int nmHttpPort,
       Clock clock, long appSubmitTime, boolean isSession, String workingDirectory,
       String [] localDirs, String[] logDirs, String clientVersion,
-      Credentials credentials, String jobUserName, AMPluginDescriptorProto pluginDescriptorProto) {
+      Credentials credentials, String jobUserName, AMPluginDescriptorProto pluginDescriptorProto, Configuration conf) {

Review Comment:
   I don't think we need a configuration here: as far as I can see the patch is about reading the default configuration and creating thread dumps from AM start to end, which is suboptimal (we don't really care about thread dumps of an idle TezAM), I was thinking about this feature a bit differently:
   
   1. we should only make thread dumps while running a DAG, so need to start dumps in startDAG and stop dumps at the end
   2. we should rely on dag config (newDAG.getConf()), this way we'll make this feature easily configurable on the fly, as the DAG's config is supposed to contain on-the-fly session props, e.g. when a hive user simply does this in beeline:
   ```
   set tez.job.periodic.thread.dump.frequency=5;
   ```



-- 
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@tez.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org