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 3:

(4 comments)

Mostly looks good to me, had a few minor comments.

http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc
File be/src/common/thread-debug-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@70
PS3, Line 70: S
nit: lowercase S


http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@72
PS3, Line 72:   // Child should set its parent_thread_name_ to the parent's 
thread_name_
Stale comment?


http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@75
PS3, Line 75: parent
parent_name for consistency with parent_* below? That would make the EXPECT_EQ 
easier to read, too.


http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h@114
PS3, Line 114:     const ThreadDebugInfo* thread_debug_info_ = nullptr;
Nit: I'm not sure how helpful it would be, but you could order everything in 
this class and the parent to have fixed width data come before variable length 
data. That way it is possible to pry it out of the binary with a debugger. Feel 
free to ignore if you think that's unnecessary.



--
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: 3
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: Wed, 07 Feb 2018 18:29:11 +0000
Gerrit-HasComments: Yes

Reply via email to