[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 15: Thank everyone for the comments! -- 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: 15 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 18 Dec 2017 15:34:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8621 ) Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack of each thread in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a core dump written by Impala, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Reviewed-on: http://gerrit.cloudera.org:8080/8621 Reviewed-by: Lars Volker Tested-by: Impala Public Jenkins --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 241 insertions(+), 2 deletions(-) Approvals: Lars Volker: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 15 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Impala Public Jenkins 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 14: Verified+1 -- 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: 14 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 18 Dec 2017 15:32:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Impala Public Jenkins 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 14: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1634/ -- 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: 14 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 18 Dec 2017 11:54:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 14: Code-Review+2 This seems to be a known issue: https://github.com/google/glog/issues/172 Carrying Tim's +2. -- 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: 14 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 18 Dec 2017 11:54:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 14: If I'm not mistaken the Jenkins verify job failed because of the warnings caused by the DCHECK_NOTNULL macros. I replaced DCHECK_NOTNULL() with DCHECK(ptr != nullptr) -- 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: 14 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 18 Dec 2017 11:43:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#14). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack of each thread in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a core dump written by Impala, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 241 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/14 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 14 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Impala Public Jenkins 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 13: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1627/ -- 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: 13 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 15 Dec 2017 22:11:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 13: Code-Review+2 Carrying Tim's +2 -- 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: 13 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 15 Dec 2017 18:30:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Impala Public Jenkins 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 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1627/ -- 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: 13 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 15 Dec 2017 18:31:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/8621/12/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/8621/12/be/src/common/thread-debug-info-test.cc@30 PS12, Line 30: ThreadDebugInfo thread_debug_info; > Not a big deal, but there's a bit more vertical whitespace in these tests t OK, removed some of them. I like using vertical whitespaces, I need some time to get used to the Impala style :) -- 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: 13 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 15 Dec 2017 09:29:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#13). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack of each thread in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a core dump written by Impala, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 241 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/13 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 13 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Tim Armstrong 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 12: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/8621/12/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/8621/12/be/src/common/thread-debug-info-test.cc@30 PS12, Line 30: Not a big deal, but there's a bit more vertical whitespace in these tests than we usually prefer. -- 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: 12 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 15 Dec 2017 00:54:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 12: Code-Review+1 Thank you for making all the changes! I think it looks very good now. -- 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: 12 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 14 Dec 2017 22:42:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@58 PS11, Line 58: > nit: remove this empty line. Done http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@73 PS11, Line 73: const int64_t front_length = THREAD_NAME_SIZE - tail_length - 4; // 4 is the length > I think if the comment doesn't fit into the end of a line it's better to pu Done 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@96 PS10, Line 96: THREAD_NAME_TAIL_LENGTH - 4; // 4 is the length of "..." and '\0' > THREAD_NAME_FRONT_LENGTH still shows up to the right, no? I think it's best Oops, now I removed it really. -- 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: 11 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 14 Dec 2017 21:38:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#12). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack of each thread in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a core dump written by Impala, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 248 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/12 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 12 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@58 PS11, Line 58: nit: remove this empty line. http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@73 PS11, Line 73: const int64_t front_length = THREAD_NAME_SIZE - tail_length - 4; // 4 is the length I think if the comment doesn't fit into the end of a line it's better to put it above the line instead of breaking it up. 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@96 PS10, Line 96: THREAD_NAME_TAIL_LENGTH - 4; // 4 is the length of "..." and '\0' > I removed THREAD_NAME_FRONT_LENGTH. THREAD_NAME_FRONT_LENGTH still shows up to the right, no? I think it's best to have NAME_SIZE and TAIL_LENGTH and then compute the rest in SetThreadName(). -- 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: 11 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 14 Dec 2017 21:07:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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
> 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
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 14 Dec 2017 13:42:43 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#11). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack of each thread in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a core dump written by Impala, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 252 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/11 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 11 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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)
I added some comments to clean it up a bit, otherwise it looks good to me.
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"
http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@22
PS10, Line 22: fully-fledged core
Replace with "core dump written by Impala"?
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
Still needed?
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?
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.
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
another file and have thread-local side effects.
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
easier to understand (and remove a member here).
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_instance_id());
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->fragment_instance_id());
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->instance_id());
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?
--
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
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Wed, 13 Dec 2017 20:00:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#10). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a fully-fledged core file, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 265 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/10 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 10 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 9: (1 comment) Thanks for the comment! I also made my namings consistent, i.e. SIZE means length of the buffer (large enough to hold the additional '\0'), LENGTH means length of the string without '\0'. http://gerrit.cloudera.org:8080/#/c/8621/8/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/8/be/src/common/thread-debug-info.h@42 PS8, Line 42: /// Only one ThreadDebugInfo object can be alive per thread at a time. > We should document whether this is copyable/movable, and if so what the sem Currently it is not copyable nor movable, but it might change in the future. Some copying/cloning functionality will be required by IMPALA-6254. Anyway, I added a comment about it. -- 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: 9 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 13 Dec 2017 10:26:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#9). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a fully-fledged core file, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 265 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/9 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Tim Armstrong 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 8: (1 comment) This is looking pretty good to me. Had one comment about comments. http://gerrit.cloudera.org:8080/#/c/8621/8/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/8/be/src/common/thread-debug-info.h@42 PS8, Line 42: // Only one ThreadDebugInfo object can be alive per thread at a time. We should document whether this is copyable/movable, and if so what the semantics are. -- 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: 8 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 12 Dec 2017 23:52:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h@81 PS5, Line 81: } > Thanks for updating this. What is the benefit of storing them separately? W Sure we can! I did it with two members because I thought it might be more explicit and I also worried a bit about the edge case when the thread name is only 1 or 2 bytes longer. -- 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: 7 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 12 Dec 2017 18:28:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#8). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a fully-fledged core file, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 264 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/8 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h@81 PS5, Line 81: } > Oh, I had a feeling that I don't understand the idea well enough, I should Thanks for updating this. What is the benefit of storing them separately? Wouldn't it be easier to print them if they were in one member (e.g. "Long Threadname with more te...001afec4)")? I suspect we'll hit this very rarely (usually our thread names are shorter) and using two members seem too complicated to me. Can we just copy len-11 from the front, 8 from the back, and add three ... in the middle? -- 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: 7 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 12 Dec 2017 17:37:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#7). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a fully-fledged core file, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 259 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/7 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 5: (2 comments) Thanks for the review! http://gerrit.cloudera.org:8080/#/c/8621/6/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/8621/6/be/src/common/thread-debug-info-test.cc@73 PS6, Line 73: uid.lo = 456; > This comment looks wrong now, but please see my remarks on truncation in th Done. http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h@81 PS5, Line 81: /// it cuts off the ending of the string > I think now it is worse than truncating at the end because we will most lik Oh, I had a feeling that I don't understand the idea well enough, I should have asked. OK, in the new patch set I store these in two different members, 'thread_name_front_' and 'thread_name_tail_' -- 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: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 12 Dec 2017 17:29:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/8621/6/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/8621/6/be/src/common/thread-debug-info-test.cc@73 PS6, Line 73: // Let's check if the stored thread name is the prefix of the long thread name This comment looks wrong now, but please see my remarks on truncation in the header first. http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h@81 PS5, Line 81: static constexpr int64_t TUNIQUE_ID_STRING_SIZE = 64; > OK, I keep the last N bytes instead of the first N bytes. I think now it is worse than truncating at the end because we will most likely cut of the most important part of the name. Instead I think we should keep everything from the front plus the last 4 or 8 characters at the end, and cut of before those. For a 300 byte name we would keep bytes 0 - 248 and 292 - 299 and cut out the bytes in between. Apologies if I didn't explain it well. -- 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: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 12 Dec 2017 16:37:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 5: (4 comments) Thanks for the review! http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h@81 PS5, Line 81: /// it cuts off the ending of the string > Would it make sense to keep the last few bytes of the string and truncate i OK, I keep the last N bytes instead of the first N bytes. http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@56 PS4, Line 56: > I feel that having separate members may make it easier to use this in debug I added this functionality to provide an easy way to add more information. But I think you are right, if someone wants to add some extra information, they will add a new data member to this class. http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@103 PS4, Line 103: > See my comment above about special purpose members. Done, I removed the text field. http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@104 PS4, Line 104: > I agree that it's helpful to keep these as a string. Is there a benefit fro I don't think there is any benefit here since we only store char buffers. Also, it is not the performance critical part of impala. I changed it to 34. -- 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: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 12 Dec 2017 16:02:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#6). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. If you have a fully-fledged core file, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s\n" thread_debug_info.instance_id Currently the thread name and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 253 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/6 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/8621/5/be/src/common/thread-debug-info.h@81 PS5, Line 81: /// it cuts off the ending of the string Would it make sense to keep the last few bytes of the string and truncate it in the middle? I feel that often long thread names would be generated and might have some counter at the end. http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@56 PS4, Line 56: > The idea was to store some description of the thread that is easy to read f I feel that having separate members may make it easier to use this in debugging tools, e.g. by enumerating all the threads and extracting particular values for each thread. Maybe Tim had some other usage in mind that requires us to have a general-purpose text field? http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@103 PS4, Line 103: > I wanted to choose a size that will be large enough for the purpose, so I c See my comment above about special purpose members. http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@104 PS4, Line 104: > I wanted it to be power of two, and also thought that it might be getting b I agree that it's helpful to keep these as a string. Is there a benefit from having the size be a power of two? http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@112 PS4, Line 112: > If you think of the name_copy variable, note that it is a std::string. It o Thanks for the clarification, that makes sense. -- 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: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 08 Dec 2017 18:41:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#5). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the ThreadDebugInfo class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently a ThreadDebugInfo object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadDebugInfo(). ThreadDebugInfo has members for the thread name and instance id. These are fixed size char buffers. ThreadDebugInfo also has a member called text, which is a char buffer that holds lines of strings. New data can be appended to the buffer by calling AddExtraInfo(line). If you have a fully-fledged core file, you can locate the ThreadDebugInfo for the current thread through the global pointer impala::thread_debug_info. In a core file that has been created from a minidump, we need to select the stack frame that allocated the ThreadDebugInfo object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s" thread_debug_info.text printf "%s\n" thread_debug_info.instance_id Currently the thread category, thread name, and instance id is stored. I created some tests in thread-debug-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-debug-info-test.cc A be/src/common/thread-debug-info.cc A be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 305 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/5 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 5: (1 comment) Thanks for the review, I implemented most change requests. http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@87 PS4, Line 87: : : : > Currently nowhere. I thought they will be useful somewhere. Maybe for prefi The getters are used by the test cases. -- 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: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 07 Dec 2017 16:54:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 4:
(9 comments)
Thanks for the comments.
I'll continue working on this after further discussions.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h
File be/src/common/thread-info.h:
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@40
PS4, Line 40: class InfoTable {
> Is this really a table? It looks more like some string buffer, or map. Mayb
Yeah, it does sound like a more reasonable name.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@56
PS4, Line 56: AddExtraInfo
> Will we always have (key,value) pairs? Since space is scarce here, I'd sugg
The idea was to store some description of the thread that is easy to read from
a debugger.
Maybe we could store lines separated by newline characters, giving more freedom
to the users.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@74
PS4, Line 74: instance_id
> nit: missing _
oops, will fix it in next commit.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@87
PS4, Line 87: const char* GetExtraInfo() const { return text_; }
: const char* GetQueryId() const { return query_id_; }
: const char* GetInstanceId() const { return instance_id_; }
: const char* GetThreadName() const { return thread_name_;
> where are these used?
Currently nowhere. I thought they will be useful somewhere. Maybe for prefixing
log messages with thread name or instance id.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@103
PS4, Line 103: static constexpr int64_t MAX_TEXT_SIZE = 4096;
> Is there a reason why you chose this size? If Breakpad estimates that the o
I wanted to choose a size that will be large enough for the purpose, so I chose
the linux page size.
But, it might be too large then, I can reduce it, or I can eliminate this free
text info feature and only the special-purpose members would remain.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@104
PS4, Line 104: static constexpr int64_t TUNIQUE_ID_STRING_SIZE = 64;
> unique id's are usually printed into 33 characters. Is there a reason you c
I wanted it to be power of two, and also thought that it might be getting
bigger in the future.
Note that query_id_ and instance_id_ are strings only because it is easier to
read during a debug session, and also if we want to prefix the log messages
with the instance id later, then maybe it's good that we have it as a string
already.
But maybe storing them as TUniqueIds would also be feasible.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@110
PS4, Line 110: char query_id_[TUNIQUE_ID_STRING_SIZE] = {};
> The fragment instance ID is actually the query id plus an index (see Impala
I didn't know that, OK, will get rid of it in the next commit.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@112
PS4, Line 112: char thread_name_[THREAD_NAME_SIZE] = {};
> We already have a copy of the thread name in the same scope as the thread-l
If you think of the name_copy variable, note that it is a std::string. It only
contains the thread name on the stack if it is small enough and Small String
Optimization is used. Otherwise, it won't appear in the minidump.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc@378
PS2, Line 378: thread_info_table->SetQueryId(fis->query_id());
> Ultimately we're interested in what query a thread is working on and it wou
OK, I agree.
I think this parent-relation can work for thread pools also. The parent-child
hierarchy would represent the task-tree, not the thread creation tree (which is
not that interesting if threads are created in a pool).
--
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: 4
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Tue, 28 Nov 2017 21:31:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Lars Volker 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 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h
File be/src/common/thread-info.h:
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@40
PS4, Line 40: class InfoTable {
Is this really a table? It looks more like some string buffer, or map. Maybe
call it ThreadDebugInfo?
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@56
PS4, Line 56: AddExtraInfo
Will we always have (key,value) pairs? Since space is scarce here, I'd suggest
storing strings and letting the clients chose how they represent them. I think
we also should provide some guidelines how this object should be used, what a
caller should do it this method returns a non-OK status, how expensive it is to
store information in this object ("can I use this in a hot loop?").
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@74
PS4, Line 74: instance_id
nit: missing _
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@87
PS4, Line 87: const char* GetExtraInfo() const { return text_; }
: const char* GetQueryId() const { return query_id_; }
: const char* GetInstanceId() const { return instance_id_; }
: const char* GetThreadName() const { return thread_name_;
where are these used?
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@103
PS4, Line 103: static constexpr int64_t MAX_TEXT_SIZE = 4096;
Is there a reason why you chose this size? If Breakpad estimates that the
overall Minidump will be too large, it reduces the amount of memory it captures
to 2KB per thread.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@104
PS4, Line 104: static constexpr int64_t TUNIQUE_ID_STRING_SIZE = 64;
unique id's are usually printed into 33 characters. Is there a reason you chose
64 here? Can you add a comment?
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@110
PS4, Line 110: char query_id_[TUNIQUE_ID_STRING_SIZE] = {};
The fragment instance ID is actually the query id plus an index (see
ImpalaInternalService.thrift), so I think we don't need to store the query id
separately.
http://gerrit.cloudera.org:8080/#/c/8621/4/be/src/common/thread-info.h@112
PS4, Line 112: char thread_name_[THREAD_NAME_SIZE] = {};
We already have a copy of the thread name in the same scope as the thread-local
instance of this class, so it doesn't seem necessary to store it again. If this
is to prevent the compiler from optimizing it out, then I think a brief comment
would help.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc@378
PS2, Line 378: thread_info_table->SetQueryId(fis->query_id());
> For starter, I fill the InfoTable on these callsites.
Ultimately we're interested in what query a thread is working on and it would
be great to extend this to threads in general purpose pools, e.g. the io mgr.
There seem to be several ways how we could do this and I think it'd be best to
have a design document and discuss possible approaches first.
--
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: 4
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Tue, 28 Nov 2017 19:16:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@61 PS2, Line 61: } > The buffers were initialized to null by the constructor's init-list. Now th When I said initialized to null, I meant they were filled with zeros. -- 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: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 23 Nov 2017 12:29:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 4:
(13 comments)
Thanks all!
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc
File be/src/common/thread-info-test.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc@55
PS2, Line 55: info_table.AddExtraInfo("extra", "info");
> I agree it would be helpful to have some more comments. Maybe at least a co
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc@55
PS2, Line 55: info_table.AddExtraInfo("extra", "info");
> please add a comment about what's going on:
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h
File be/src/common/thread-info.h:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@37
PS2, Line 37: /// function 'GetThreadInfoTable()'.
> "Until this object lives" -> "While this object is alive"
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@41
PS2, Line 41: public:
> This is the only usage of boost:noncopyable I've seen in our code base. We
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@41
PS2, Line 41: public:
> Yeah DISALLOW_COPY_AND_ASSIGN is the canonical way to do it in Impala (for
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@42
PS2, Line 42: // Only one InfoTable object can be alive per thread at a time.
> We've generally been moving to using the new inline initialisation syntax f
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@43
PS2, Line 43: InfoTable() {
> We should comment that you're only allowed to create one of these per threa
Yeah it is a bit twisted because we need to create this object on the stack
(for minidumps), then make the global pointer point to it.
I added a comment about it.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@61
PS2, Line 61: }
> Would line.copy(text + text_size, line.length()) be clearer here?
The buffers were initialized to null by the constructor's init-list. Now they
are initialized to null by the inline member initializers.
I switched to the member copy.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@101
PS2, Line 101: dInfo(
> We generally use int64_t for byte sizes in the codebase. There are argument
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@105
PS2, Line 105: static constexpr int64_t THREAD_NAME_SIZE = 256;
> The naming convention for member variables is to include a _ suffix. In thi
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@108
PS2, Line 108: int64_t text_size_ = 0;
> You're choosing to store this as a string intentionally?
I thought this makes easier to display it during a debug session.
Also, if we want to use it for prepending log lines, it might be more efficient
to have it as a string already.
But I can switch to TUniqueId if needed.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@115
PS2, Line 115: };
> By convention we generally use pointers if arguments or return values are m
Done
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc@378
PS2, Line 378: thread_info_table->SetQueryId(fis->query_id());
> We should also do this for other threads running in the context of the quer
For starter, I fill the InfoTable on these callsites.
I am thinking about extending the Thread class, making it aware of InfoTables.
Like a child thread's InfoTable is the copy of the parent InfoTable at the
beginning.
Or, I extend the InfoTable to have a pointer to the parent's InfoTable. This
way we would have a tree-like structure of how the threads created each other.
This works if threads always join, a bit trickier if threads can be detached.
What do you think about it?
--
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: 4
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 23 Nov 2017 12:26:27 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#4). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the InfoTable class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently an InfoTable object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadInfoTable(). InfoTable has members for the thread name, query id, and instance id. These are fixed size char buffers. InfoTable also has a member called text, which is a char buffer that holds lines of strings. A line has a format of = . New data can be appended to the buffer by calling AddExtraInfo(key, value). If you have a fully-fledged core file, you can locate the InfoTable for the current thread through the global pointer impala::thread_info_table. In a core file that has been created from a minidump, we need to select the stack frame that allocated the InfoTable object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s" thread_info_table.text printf "%s\n" thread_info_table.query_id Currently the thread category, thread name, query id, and instance id is stored. I created some tests in thread-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-info-test.cc A be/src/common/thread-info.cc A be/src/common/thread-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/query-state.cc M be/src/util/thread.cc 9 files changed, 322 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/4 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#3). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the InfoTable class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently an InfoTable object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadInfoTable(). InfoTable has members for the thread name, query id, and instance id. These are fixed size char buffers. InfoTable also has a member called text, which is a char buffer that holds lines of strings. A line has a format of = . New data can be appended to the buffer by calling AddExtraInfo(key, value). If you have a fully-fledged core file, you can locate the InfoTable for the current thread through the global pointer impala::thread_info_table. In a core file that has been created from a minidump, we need to select the stack frame that allocated the InfoTable object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s" thread_info_table.text printf "%s\n" thread_info_table.query_id Currently the thread category, thread name, query id, and instance id is stored. I created some tests in thread-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-info-test.cc A be/src/common/thread-info.cc A be/src/common/thread-info.h M be/src/runtime/query-state.cc M be/src/util/thread.cc 6 files changed, 303 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/3 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Tim Armstrong 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 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc
File be/src/common/thread-info-test.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc@55
PS2, Line 55: Status status = info_table.AddExtraInfo("extra", "info");
> please add a comment about what's going on:
I agree it would be helpful to have some more comments. Maybe at least a
comment on each test to explain what it is testing at a high-level, then
possible comments for any tricky details of the tests themselves.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h
File be/src/common/thread-info.h:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@41
PS2, Line 41: class InfoTable : boost::noncopyable {
> This is the only usage of boost:noncopyable I've seen in our code base. We
Yeah DISALLOW_COPY_AND_ASSIGN is the canonical way to do it in Impala (for
better or worse).
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@42
PS2, Line 42: public:
We've generally been moving to using the new inline initialisation syntax for
member variables if they're constant, or we're just calling the default
constructor. I.e.
text_size = 0, etc.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@101
PS2, Line 101: size_t
We generally use int64_t for byte sizes in the codebase. There are arguments
for either int64_t or size_t, but we've settled on int64_t in Impala because
it's large enough to hold any practical size and we avoid any
hard-to-reason-about edge cases with mixed signed/unsigned expressions.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@105
PS2, Line 105: char text[MAX_TEXT_SIZE];
The naming convention for member variables is to include a _ suffix. In this
case we're following the google style guide:
https://google.github.io/styleguide/cppguide.html#Variable_Names
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@115
PS2, Line 115: InfoTable& GetThreadInfoTable();
By convention we generally use pointers if arguments or return values are meant
to be mutable.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc@378
PS2, Line 378: thread_info_table.SetQueryId(fis->query_id());
We should also do this for other threads running in the context of the
query/fragment.
The fragment thread can spawn off threads in a couple of places:
BlockingJoinNode::ProcessBuildInputAndOpenProbe() and
HdfsScanNode::ThreadTokenAvailableCb().
FragmentInstanceState::Prepare() also spawns off a per-fragment thread,
although I believe at some point that will become a per-query thread.
I guess we could also automatically inherit query_id and fragment_id from the
parent thread, if that works out sanely, or maybe we should have a special
Thread::Create() for fragment/query threads that takes additional arguments.
Mainly it would be nice to have a pattern that makes it harder to accidentally
avoid adding the appropriate info.
--
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: 2
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 23 Nov 2017 01:20:29 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Philip Zeyliger 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 2:
(6 comments)
Thanks for the updates. Some more comments.
I'd advise you to get Tim/Lars to look at this before reacting too much to my
comments, since there are a bunch of things here that I'm insufficiently
familiar with to review wisely. But I like the direction it's going!
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc
File be/src/common/thread-info-test.cc:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc@55
PS2, Line 55: Status status = info_table.AddExtraInfo("extra", "info");
please add a comment about what's going on:
// We've exhausted our allocated space by now, so AddExtraInfo should be
failing.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h
File be/src/common/thread-info.h:
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@37
PS2, Line 37: /// it in minidumps. Until this object lives, it is available
through the global
"Until this object lives" -> "While this object is alive"
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@41
PS2, Line 41: class InfoTable : boost::noncopyable {
This is the only usage of boost:noncopyable I've seen in our code base. We
should check that it's the style of this sort of thing we want.
I think DISALLOW_COPY_AND_ASSIGN does something very similar and is present in
our code base. It's defined in be/src/gutil/macros.h.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@43
PS2, Line 43: InfoTable() : text(), text_size(0), query_id(), instance_id(),
thread_name() {
We should comment that you're only allowed to create one of these per thread at
a time. I had to think through how the constructor/destructor update a thread
local. (I had assumed it would work the other way, with the thread_local being
maintained by the cpde that's creating these , but that may just be the Java in
me speaking.)
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@61
PS2, Line 61: std::copy(line.begin(), line.end(), text + text_size);
Would line.copy(text + text_size, line.length()) be clearer here?
I don't think anything is null-terminating these, so printf in the debugger may
have a bunch of junk in the remaining buffer. We should either null-terminate
the strings here, or, perhaps better, zero out the bytes when initialized.
http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@108
PS2, Line 108: char query_id[TUNIQUE_ID_STRING_SIZE];
You're choosing to store this as a string intentionally?
--
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: 2
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Wed, 22 Nov 2017 17:56:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Hello Lars Volker, Philip Zeyliger, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8621 to look at the new patch set (#2). Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the InfoTable class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently an InfoTable object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function GetThreadInfoTable(). InfoTable has members for the thread name, query id, and instance id. These are fixed size char buffers. InfoTable also has a member called text, which is a char buffer that holds lines of strings. A line has a format of = . New data can be appended to the buffer by calling AddExtraInfo(key, value). If you have a fully-fledged core file, you can locate the InfoTable for the current thread through the global pointer impala::thread_info_table. In a core file that has been created from a minidump, we need to select the stack frame that allocated the InfoTable object in order to inspect it. It is currently allocated in Thread::SuperviseThread(). We can use printf in gdb to print the members, e.g.: printf "%s" thread_info_table.text printf "%s\n" thread_info_table.query_id Currently the thread category, thread name, query id, and instance id is stored. I created some tests in thread-info-test.cc Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-info-test.cc A be/src/common/thread-info.cc A be/src/common/thread-info.h M be/src/runtime/query-state.cc M be/src/util/thread.cc 6 files changed, 285 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/2 -- 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: newpatchset Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
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 1: (2 comments) Thanks for the review. I added SetThreadName(), SetQueryId(), and SetInstanceId() to InfoTable. They have their separate buffer on the stack. > "how hard will it be to visit a /threads debug page and see what all the > threads are up to?" If you have a complete core file (one that is not generated from a minidump), one can traverse all the threads and inspect the global impala::thread_info_table object and retrieve the needed information. If you have a core that is created from a minidump, it is only a little bit more complicated. During thread traversion one need to select the stack frame that has the InfoTable object (currently it is Thread::SuperviseThread()). After the needed information can be retrieved from the InfoTable object. > "Have you tried pulling one of these out in a minidump?" I used minidumps, but I converted them to core files because the tooling for minidumps are not that user friendly / feature rich. > RegisterAppMemory Yes, this can be used, but then I would need to call RegisterAppMemory on every thread creation, and UnregisterAppMemory on every thread destruction. It is not that big a deal, since I can put this logic into InfoTable's constructor/destructor pairs. I would also need to make the breakpad ExceptionHandler object accessible for InfoTable, and synchronize the accesses to it between threads. With stack allocation, everything is automatically included in the minidump. If you say we cannot afford having this object on the stack, I rewrite it to use the heap. > testing I created backend tests in thread-info-test.cc http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@70 PS1, Line 70: /// Call this global function to add information about the current thread > Should we document that you should avoid information that's sensitive here? I removed this function, now only the member AddExtraInfo() can be used to add unstructured text info to the InfoTable. I added a comment there that it is not for sensitive data. http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@73 PS1, Line 73: /// is a no-op. > It's weird to me that this can be a no-op if there's no object, but it fail I removed this function, now we cannot call InfoTable::AddExtraInfo() without an object. -- 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: 1 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 22 Nov 2017 16:14:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Philip Zeyliger 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 1: (2 comments) On the JIRA, Dan asked about this being a basis for things like prepending log messages with query ids and so on. The unstructured element of just appending to a 4k block of memory may make that hard. Did you consider doing a more structured system? (e.g., exposing setQueryId() and setThreadName() and having those store values or offsets/lengths into the buffer.) There are trade-offs of course. Relatedly, how hard will it be to visit a /threads debug page and see what all the threads are up to? Have you tried pulling one of these out in a minidump? The commit message mentioned gdb, but minidumps have their own tooling. For what it's worth, if we wanted to avoid the stack allocation, it looks like the following API exists: // Register a block of memory of length bytes starting at address ptr // to be copied to the minidump when a crash happens. void RegisterAppMemory(void* ptr, size_t length); I'd encourage you to write a test. There's a little bit of byte-counting in making sure we don't overflow the array. I think it's right, but should be testable. http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@70 PS1, Line 70: /// Call this global function to add information about the current thread Should we document that you should avoid information that's sensitive here? i.e., we should avoid things like query text because we're trying to avoid them in minidumps? Or not? http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@73 PS1, Line 73: /// is a no-op. It's weird to me that this can be a no-op if there's no object, but it fails at line 48 if it's full. -- 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: 1 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 21 Nov 2017 22:55:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8621 Change subject: IMPALA-3703: Store query context in thread-local variables .. IMPALA-3703: Store query context in thread-local variables This commit introduces the InfoTable class which can hold information about the current thread that can be useful in debug sessions. It needs to be allocated on the stack in order to include it to minidumps. Currently an InfoTable object is created in Thread::SuperviseThread. This object is available in all the child stack frames through the global function AddThreadInfo(key, value). In a core file we can select the stack frame of SuperviseThread, and issue 'print "%s", thread_info_table.text' to display the information stored in the InfoTable. Currently the thread category, thread name, query id, and instance id is stored. Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 --- M be/src/common/CMakeLists.txt A be/src/common/thread-info.cc A be/src/common/thread-info.h M be/src/runtime/query-state.cc M be/src/util/thread.cc 5 files changed, 129 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/1 -- 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: newchange Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy
