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 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"? 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 (parent == nullptr) return; http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@93 PS2, Line 93: t nit: capital T 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 parent_tdi. Maybe "ParentThreadInfo" or "ParentInfo" would be more clear? 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 thread's data here. Could we instead store two things: a numerical ID for minidump based analysis that allows to find the next thread to inspect. This would likely be the system thread id. Secondly we could store a pointer to the parent thread's info. However, that would break if the parent exits before the child. As an alternative, can we wrap the relevant fields in some internal struct and then have two members, "S thread_info;" and "S parent_info;"? This way we keep all data twice, but we know for sure that we can traverse the thread ancestry two levels deep, even if the parent has exited. 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. 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 strictly an input parameter and we order them as inputs, then output. -- 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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 01 Feb 2018 22:54:11 +0000 Gerrit-HasComments: Yes
