Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12129 )
Change subject: IMPALA-6664: Tag log statements with fragment or query ids. ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/common/thread-debug-info.h@a94 PS3, Line 94: We should copy parent_.thread_name_, or remove it from struct 'ParentInfo'. http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/thread-debug-info.h@a116 PS2, Line 116: > Yep, I'll wait for Zoltan's input. Yeah, the idea was to make it readable when debugging a minidump or core file and you can't invoke PrintId() from GDB. But since PrintId() just prints the hex of members 'hi' and 'lo', it should be fine. But we also need to update 'lib/python/impala_py_lib/gdb/impala-gdb.py' because currently it expects 'instance_id_' to be a string. It also breaks the unit tests in 'thread-debug-info-test.cc'. http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/exec/hdfs-scan-node-base.cc@37 PS3, Line 37: #include "common/thread-debug-info.h" nit: do we need this include? Might be better to include it in hdfs-scan-node.cc http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/exec/hdfs-scan-node.cc@325 PS3, Line 325: ThreadDebugInfo* parent_thread_debug_info = GetThreadDebugInfo(); : DCHECK(parent_thread_debug_info != nullptr); : auto fn = [this, first_thread, scanner_thread_reservation, : parent_thread_debug_info]() { : GetThreadDebugInfo()->SetParentInfo(parent_thread_debug_info); Since we use Thread::Create() later, https://github.com/apache/impala/blob/274e96bd147b5d91872c441c3a600fa8d5295bbe/be/src/util/thread.cc#L317 and https://github.com/apache/impala/blob/274e96bd147b5d91872c441c3a600fa8d5295bbe/be/src/util/thread.cc#L351 should do this automatically. Doesn't it happen? http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/12129/3/be/src/runtime/io/disk-io-mgr.cc@424 PS3, Line 424: // We are now working on behalf of a query, so set thread state appropriately. : GetThreadDebugInfo()->SetQueryId(worker_context->query_id()); : GetThreadDebugInfo()->SetInstanceId(worker_context->instance_id()); Fyi, IMPALA-6254 and IMPALA-6417 is about to make this automated. Maybe we could add a TODO here and mention those Jiras. -- To view, visit http://gerrit.cloudera.org:8080/12129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6634ef9d1a7346339f24f2d40a7a3aa36a535da8 Gerrit-Change-Number: 12129 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@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-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Wed, 09 Jan 2019 11:25:26 +0000 Gerrit-HasComments: Yes
