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