impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
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. ( 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 <boroknagyz@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2017 17:56:00 +0000
Gerrit-HasComments: Yes

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