Zoltan Borok-Nagy 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) Thanks for the comments. I checked the callsites of Thread::Create() and the name is always set explicitly. If it weren't, then Thread::SuperviseThread() would create a unique thread name with the system thread id. Therefore, as far as parent threads outlive their child threads we can restore the full thread trees. I think this "2 gen by Instance Worker Foo" might be more relevant for the general purpose thread pools where we just submit work items and don't set any names at all. 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 Yeah it's a good idea. I created a nested struct called ParentTdi for the parent fields, to keep things organized. 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? Th No, I think we can allow and ignore nullptrs. 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 i Yes. I checked all these places with GDB to test if instance id propagates to the child TDI. 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 file Oops, I think I've copied the new signature from the .cc I got wonder how did it compile, then I realized that I've included common/names.h in thread-debug-info.h. Removed the include. 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 Added comment, and also made the parameter a pointer to const. 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 Removed the check since ExtractInfoFromParent() now accepts nullptrs as well. And since it is an inline function we don't save the cost of a function call with this check. -- 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 <borokna...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 01 Feb 2018 13:28:36 +0000 Gerrit-HasComments: Yes
