Mailing-List: contact reviews-help@impala.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711231226.vANCQRlG029489@ip-10-146-233-104.ec2.internal>
Date: Thu, 23 Nov 2017 12:26:27 +0000
From: "Zoltan Borok-Nagy (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>, Lars Volker <lv@cloudera.com>,
        Philip Zeyliger <philip@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3703=3A_Store_query_context_in_thread-local_variables=0A?=
In-Reply-To: <gerrit.1511272401000.I566f7f1db5117c498e86e0bd05b33bdcff624609@gerrit.cloudera.org>
References: <gerrit.1511272401000.I566f7f1db5117c498e86e0bd05b33bdcff624609@gerrit.cloudera.org>
Reply-To: boroknagyz@cloudera.com, impala-cr@cloudera.com, lv@cloudera.com,
        marcelk@gmail.com, tarmstrong@cloudera.com, dhecht@cloudera.com,
        philip@cloudera.com, reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="8pGgxaYU5tU="; charset=UTF-8
archived-at: Thu, 23 Nov 2017 12:26:34 -0000

--8pGgxaYU5tU=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Zoltan Borok-Nagy has posted comments on this change=2E ( http://gerrit=2Ec=
loudera=2Eorg:8080/8621 )

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


Patch Set 4:

(13 comments)

Thanks all!

http://gerrit=2Ecl=
oudera=2Eorg:8080/#/c/8621/2/be/src/common/thread-info-test=2Ecc
File be/sr=
c/common/thread-info-test=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/86=
21/2/be/src/common/thread-info-test=2Ecc@55
PS2, Line 55:     info_table=2E=
AddExtraInfo("extra", "info");
> I agree it would be helpful to have some m=
ore comments=2E Maybe at least a co
Done


http://gerrit=2Ecloudera=2Eorg:8=
080/#/c/8621/2/be/src/common/thread-info-test=2Ecc@55
PS2, Line 55:     inf=
o_table=2EAddExtraInfo("extra", "info");
> please add a comment about what'=
s going on:
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/2/be/src/co=
mmon/thread-info=2Eh
File be/src/common/thread-info=2Eh:

http://gerrit=2Ec=
loudera=2Eorg:8080/#/c/8621/2/be/src/common/thread-info=2Eh@37
PS2, Line 37=
: /// function 'GetThreadInfoTable()'=2E
> "Until this object lives" -> "Wh=
ile this object is alive"
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/86=
21/2/be/src/common/thread-info=2Eh@41
PS2, Line 41: public:
> This is the o=
nly usage of boost:noncopyable I've seen in our code base=2E We 
Done


htt=
p://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/2/be/src/common/thread-info=2Eh@4=
1
PS2, Line 41: public:
> Yeah DISALLOW_COPY_AND_ASSIGN is the canonical wa=
y to do it in Impala (for 
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8=
621/2/be/src/common/thread-info=2Eh@42
PS2, Line 42:   // Only one InfoTabl=
e object can be alive per thread at a time=2E
> We've generally been moving=
 to using the new inline initialisation syntax f
Done


http://gerrit=2Eclo=
udera=2Eorg:8080/#/c/8621/2/be/src/common/thread-info=2Eh@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=2E
I added a comment about it=2E


http://gerrit=2Ecloudera=2Eorg:8080/#=
/c/8621/2/be/src/common/thread-info=2Eh@61
PS2, Line 61:     }
> Would line=
=2Ecopy(text + text_size, line=2Elength()) be clearer here?
The buffers wer=
e initialized to null by the constructor's init-list=2E Now they are initia=
lized to null by the inline member initializers=2E

I switched to the membe=
r copy=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/2/be/src/common/th=
read-info=2Eh@101
PS2, Line 101: dInfo(
> We generally use int64_t for byte=
 sizes in the codebase=2E There are argument
Done


http://gerrit=2Eclouder=
a=2Eorg:8080/#/c/8621/2/be/src/common/thread-info=2Eh@105
PS2, Line 105:   =
static constexpr int64_t THREAD_NAME_SIZE =3D 256;
> The naming convention =
for member variables is to include a _ suffix=2E In thi
Done


http://gerri=
t=2Ecloudera=2Eorg:8080/#/c/8621/2/be/src/common/thread-info=2Eh@108
PS2, L=
ine 108:   int64_t text_size_ =3D 0;
> You're choosing to store this as a s=
tring intentionally?
I thought this makes easier to display it during a deb=
ug session=2E
Also, if we want to use it for prepending log lines, it might=
 be more efficient to have it as a string already=2E
But I can switch to TU=
niqueId if needed=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/2/be/sr=
c/common/thread-info=2Eh@115
PS2, Line 115: };
> By convention we generally=
 use pointers if arguments or return values are m
Done


http://gerrit=2Ecl=
oudera=2Eorg:8080/#/c/8621/2/be/src/runtime/query-state=2Ecc
File be/src/ru=
ntime/query-state=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/2/be/=
src/runtime/query-state=2Ecc@378
PS2, Line 378:   thread_info_table->SetQue=
ryId(fis->query_id());
> We should also do this for other threads running i=
n the context of the quer
For starter, I fill the InfoTable on these callsi=
tes=2E

I am thinking about extending the Thread class, making it aware of =
InfoTables=2E Like a child thread's InfoTable is the copy of the parent Inf=
oTable at the beginning=2E
Or, I extend the InfoTable to have a pointer to =
the parent's InfoTable=2E This way we would have a tree-like structure of h=
ow the threads created each other=2E This works if threads always join, a b=
it trickier if threads can be detached=2E

What do you think about it?


-=
- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/8621
To unsubscribe, =
visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerrit-Project: Impala-=
ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56=
6f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-Pa=
tchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <boroknagyz@cloudera=2Ecom>
Gerri=
t-Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Gerrit-Reviewer: Lars Volker =
<lv@cloudera=2Ecom>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera=2Ecom=
>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Reviewe=
r: Zoltan Borok-Nagy <boroknagyz@cloudera=2Ecom>
Gerrit-Comment-Date: Thu, =
23 Nov 2017 12:26:27 +0000
Gerrit-HasComments: Yes

--8pGgxaYU5tU=--