Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 )
Change subject: IMPALA-6416: extend Thread::Create to track instance id ...................................................................... Patch Set 1: (6 comments) I left some inline comments. I feel it would be better to find a way to propagate thread names indefinitely, compared to propagating them for one generation (which is my understanding of this patch). Currently it allows us to remove the call to the explicit setter in the scan node, but one generation further the same mechanism will not work anymore. Maybe we could think of deriving names automatically (if they are not explicitly set), e.g. by prepending the parent name with an increasing counter. If "Instance Worker Foo" creates a child, then its name could be "1 gen by Instance Worker Foo", and if this child spawns a thread w/out an explicit name, it could be called "2 gen by Instance Worker Foo" or similar. This way we could programatically capture full thread trees. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h@70 PS1, Line 70: void ExtractInfoFromParent(const ThreadDebugInfo* parent) { Have you considered storing the parent thread ID (possibly both system and boost) in the ThreadDebugInfo, too? While every thread had its own Info it seemed superflous to store it again, but for the parent threads it might be helpful, especially when thinking of programmatic ways to analyze the thread graph. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h@71 PS1, Line 71: DCHECK(parent != nullptr); Is there a benefit of not allowing nullptr vs. allowing and ignoring it? The latter could make the calling code a bit more concise, but I may be missing something. If you keep the requirement that parent != nullptr, you could consider making this a static factory that creates and pre-populates a TDI. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/exec/hdfs-scan-node.cc@a353 PS1, Line 353: My understanding is that this is no longer needed because the information is now kept in the parent TDI. Is this correct? http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h File be/src/util/thread.h: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h@183 PS1, Line 183: static void SuperviseThread(const string& name, const string& category, Our convention is usually to keep the std:: namespace prefix in header files but drop it in .cc files. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h@185 PS1, Line 185: parent_thread_info The comment should explain what this parameter does (and whether it can be null), especially since we usually pass input parameters as const& and output parameters as non-const ptrs. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.cc File be/src/util/thread.cc: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.cc@350 PS1, Line 350: if (parent_thread_info) thread_debug_info.ExtractInfoFromParent(parent_thread_info); We usually make the comparison to != nullptr explicit to make it more clear what is happening. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 25 Jan 2018 00:24:01 +0000 Gerrit-HasComments: Yes
