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 <borokna...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@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: Wed, 14 Feb 2018 17:32:14 +0000
Gerrit-HasComments: Yes

Reply via email to