Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8621 )
Change subject: IMPALA-3703: Store query context in thread-local variables ...................................................................... Patch Set 10: (11 comments) Thank you very much! http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@11 PS10, Line 11: in debug sessions. It needs to be allocated on the stack in > mention "on the stack of each thread" Done http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@22 PS10, Line 22: fully-fledged core > Replace with "core dump written by Impala"? Done http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@20 PS10, Line 20: #include <vector> > Still needed? No, removed it and others as well. http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@30 PS10, Line 30: using boost::split; > still needed? No, removed it. http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@83 PS10, Line 83: void PrintUniqueIdToMember(const TUniqueId& id, char* member) { > We could inline this now as it's only used once. Done http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@90 PS10, Line 90: static void InitializeThreadDebugInfo(ThreadDebugInfo* thread_debug_info); > I think these could benefit from a brief comment since they're defined in a Added short comments about them. http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@96 PS10, Line 96: static constexpr int64_t THREAD_NAME_FRONT_LENGTH = THREAD_NAME_SIZE - > I think moving this computation to SetThreadName() would make it slightly e I removed THREAD_NAME_FRONT_LENGTH. Should I remove THREAD_NAME_TAIL_LENGTH as well? http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc File be/src/exec/blocking-join-node.cc: http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc@199 PS10, Line 199: ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo(); > You could also do GetThreadDebugInfo()->SetInstanceId(state->fragment_insta yes, previously I set more member (query id). Done. http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc@353 PS10, Line 353: ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo(); > You could shorten it a bit to GetThreadDebugInfo()->SetInstanceId(state->fr Done http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc@232 PS10, Line 232: ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo(); : thread_debug_info->SetInstanceId(this->instance_id()); > You could shorten this to GetThreadDebugInfo()->SetInstanceId(this->instanc Done http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc@377 PS10, Line 377: thread_debug_info > Inline the call to GetThreadDebugInfo() here, too? Done -- To view, visit http://gerrit.cloudera.org:8080/8621 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 10 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 14 Dec 2017 13:42:43 +0000 Gerrit-HasComments: Yes
