impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Date Wed, 22 Nov 2017 17:56:00 GMT
Philip Zeyliger has posted comments on this change. (

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

Patch Set 2:


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!
File be/src/common/
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.
File be/src/common/thread-info.h:
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"
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.
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.)
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.
PS2, Line 108:   char query_id[TUNIQUE_ID_STRING_SIZE];
You're choosing to store this as a string intentionally?

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: 2
Gerrit-Owner: Zoltan Borok-Nagy <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Wed, 22 Nov 2017 17:56:00 +0000
Gerrit-HasComments: Yes

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