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 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@115 PS4, Line 115: system_thread_id_ > Are you saying that thread_debug_info_ pointer may point to stray > memory? Yes, I think that can potentially happen. > If that's the case, maybe we shouldn't keep that pointer > but instead have a way to traverse TDIs and find it if still > exists? We can use the system thread id to find the parent thread and then its TDI if it still exists. I also added a copy of the parent thread name. Even if the parent thread exits, at least we will now what was its name. We can also check if parent thread name equals to the TDI's thread name that we found via the system thread id. If they're not the same, we'll know that the parent thread was re-cycled in some way. http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@119 PS4, Line 119: boost::thread::id boost_thread_id_; : int64_t system_thread_id_ = 0; > I'm not sure what you mean by "switch to the parent thread > quickly". I was just thinking of a GDB session. GDB prints out the system thread id for each thread, so we can traverse the threads quickly if we know the ID. It can be also useful if we write a script for that and we don't want to iterate through all the threads. > I don't think I fully understand the tradeoffs. Just seems > confusing to have multiple IDs and I don't understand how each of > these fields helps in debugging. Perhaps some comments attached to > them explaining how one can use them for debugging would help? I removed boost::thread::id, and added the parent's thread name. I think it is more clear, and knowing the parent thread name can be useful on its own. And we can also use it to verify the parent thread. -- 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: 4 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 14 Feb 2018 17:32:14 +0000 Gerrit-HasComments: Yes
