impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Borok-Nagy (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Date Thu, 23 Nov 2017 12:26:27 GMT
Zoltan Borok-Nagy has posted comments on this change. (

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

Patch Set 4:


Thanks all!
File be/src/common/
PS2, Line 55:     info_table.AddExtraInfo("extra", "info");
> I agree it would be helpful to have some more comments. Maybe at least a co
PS2, Line 55:     info_table.AddExtraInfo("extra", "info");
> please add a comment about what's going on:
File be/src/common/thread-info.h:
PS2, Line 37: /// function 'GetThreadInfoTable()'.
> "Until this object lives" -> "While this object is alive"
PS2, Line 41: public:
> This is the only usage of boost:noncopyable I've seen in our code base. We 
PS2, Line 41: public:
> Yeah DISALLOW_COPY_AND_ASSIGN is the canonical way to do it in Impala (for 
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
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.
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.
PS2, Line 101: dInfo(
> We generally use int64_t for byte sizes in the codebase. There are argument
PS2, Line 105:   static constexpr int64_t THREAD_NAME_SIZE = 256;
> The naming convention for member variables is to include a _ suffix. In thi
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.
PS2, Line 115: };
> By convention we generally use pointers if arguments or return values are m
File be/src/runtime/
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
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Thu, 23 Nov 2017 12:26:27 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message