You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2017/11/21 13:53:21 UTC

[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 <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 09:29:49 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2017 21:31:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 18:30:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:55:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:42:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:07:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 17:37:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 15:32:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 16:54:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Nov 2017 12:29:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Nov 2017 01:20:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2017 17:56:00 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <lv...@cloudera.com>
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2017 19:16:01 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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
<key> = <value>. 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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 11:54:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 22:11:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2017 10:26:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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
<key> = <value>. 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 <bo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2017 16:14:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 16:37:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <vector>
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2017 20:00:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 11:54:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 11:43:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 00:54:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Nov 2017 12:26:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 18:41:24 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 18:31:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8621 )

Change subject: IMPALA-3703: Store query context in thread-local variables
......................................................................


Patch Set 10:

(11 comments)

Thank you very much!

http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@11
PS10, Line 11: in debug sessions. It needs to be allocated on the stack in
> mention "on the stack of each thread"
Done


http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@22
PS10, Line 22: fully-fledged core
> Replace with "core dump written by Impala"?
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc
File be/src/common/thread-debug-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@20
PS10, Line 20: #include <vector>
> Still needed?
No, removed it and others as well.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@30
PS10, Line 30: using boost::split;
> still needed?
No, removed it.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@83
PS10, Line 83:   void PrintUniqueIdToMember(const TUniqueId& id, char* member) {
> We could inline this now as it's only used once.
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@90
PS10, Line 90:   static void InitializeThreadDebugInfo(ThreadDebugInfo* thread_debug_info);
> I think these could benefit from a brief comment since they're defined in a
Added short comments about them.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@96
PS10, Line 96:   static constexpr int64_t THREAD_NAME_FRONT_LENGTH = THREAD_NAME_SIZE -
> I think moving this computation to SetThreadName() would make it slightly e
I removed THREAD_NAME_FRONT_LENGTH.
Should I remove THREAD_NAME_TAIL_LENGTH as well?


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc@199
PS10, Line 199:           ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
> You could also do GetThreadDebugInfo()->SetInstanceId(state->fragment_insta
yes, previously I set more member (query id).

Done.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc@353
PS10, Line 353:       ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
> You could shorten it a bit to GetThreadDebugInfo()->SetInstanceId(state->fr
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc@232
PS10, Line 232:           ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
              :           thread_debug_info->SetInstanceId(this->instance_id());
> You could shorten this to GetThreadDebugInfo()->SetInstanceId(this->instanc
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc@377
PS10, Line 377: thread_debug_info
> Inline the call to GetThreadDebugInfo() here, too?
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/8621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2017 13:42:43 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 17:29:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 15:34:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 23:52:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 18:28:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 16:02:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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
<key> = <value>. 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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:38:16 +0000
Gerrit-HasComments: Yes