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 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@79 PS2, Line 79: ExtractInfoFromParent > Extract... reads like something will be returned. How about "SetParentInfo" yeah I like that. Done. http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@80 PS2, Line 80: if (parent != nullptr) { > We usually early return instead of scoping the whole function, i.e. if (par Done http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@93 PS2, Line 93: t > nit: capital T Done http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@121 PS2, Line 121: struct ParentTdi { > Can you add a comment to this struct, too? The test has a ThreadDebugInfo p Added comment, and renamed it to "ParentInfo". http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@124 PS2, Line 124: char thread_name_[THREAD_NAME_SIZE] = {}; > I wonder if we can streamline the selective duplication of the parent threa I chose the first alternative, i.e. to store the system thread id and a pointer to the parent TDI. If it is a common case that the parent exits, we can switch to the second alternative, but also adding a pointer to the parent for convenience. http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@128 PS2, Line 128: > nit: I think this newline and the next one don't add much to readability. Done http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/util/thread.h File be/src/util/thread.h: http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/util/thread.h@191 PS2, Line 191: const ThreadDebugInfo* parent_thread_info > This should go to the front (behind category or functor) now since it's str oh I see. I placed it behind functor. -- 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: 2 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: Fri, 02 Feb 2018 14:38:14 +0000 Gerrit-HasComments: Yes
