Mailing-List: contact reviews-help@impala.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711282131.vASLVpBk015086@ip-10-146-233-104.ec2.internal>
Date: Tue, 28 Nov 2017 21:31:50 +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="mc07qZ37euE="; charset=UTF-8
archived-at: Tue, 28 Nov 2017 21:31:57 -0000

--mc07qZ37euE=
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:

(9 comments)

Thanks for the comments=2E
I'll =
continue working on this after further discussions=2E

http://gerrit=2Eclou=
dera=2Eorg:8080/#/c/8621/4/be/src/common/thread-info=2Eh
File be/src/common=
/thread-info=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/4/be/src/co=
mmon/thread-info=2Eh@40
PS4, Line 40: class InfoTable {
> Is this really a =
table? It looks more like some string buffer, or map=2E Mayb
Yeah, it does =
sound like a more reasonable name=2E


http://gerrit=2Ecloudera=2Eorg:8080/=
#/c/8621/4/be/src/common/thread-info=2Eh@56
PS4, Line 56: AddExtraInfo
> Wi=
ll we always have (key,value) pairs? Since space is scarce here, I'd sugg
T=
he idea was to store some description of the thread that is easy to read fr=
om a debugger=2E
Maybe we could store lines separated by newline characters=
, giving more freedom to the users=2E


http://gerrit=2Ecloudera=2Eorg:8080=
/#/c/8621/4/be/src/common/thread-info=2Eh@74
PS4, Line 74: instance_id
> ni=
t: missing _
oops, will fix it in next commit=2E


http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/8621/4/be/src/common/thread-info=2Eh@87
PS4, Line 87:  cons=
t char* GetExtraInfo() const { return text_; }
            :   const char* =
GetQueryId() const { return query_id_; }
            :   const char* GetIns=
tanceId() const { return instance_id_; }
            :   const char* GetThr=
eadName() const { return thread_name_; 
> where are these used?
Currently n=
owhere=2E I thought they will be useful somewhere=2E Maybe for prefixing lo=
g messages with thread name or instance id=2E


http://gerrit=2Ecloudera=2E=
org:8080/#/c/8621/4/be/src/common/thread-info=2Eh@103
PS4, Line 103:   stat=
ic constexpr int64_t MAX_TEXT_SIZE =3D 4096;
> Is there a reason why you ch=
ose this size? If Breakpad estimates that the o
I wanted to choose a size t=
hat will be large enough for the purpose, so I chose the linux page size=2E=

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=2E=


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/4/be/src/common/thread-info=
=2Eh@104
PS4, Line 104:   static constexpr int64_t TUNIQUE_ID_STRING_SIZE =
=3D 64;
> unique id's are usually printed into 33 characters=2E 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=2E
Note that query_id_ and instance_id_ ar=
e strings only because it is easier to read during a debug session, and als=
o if we want to prefix the log messages with the instance id later, then ma=
ybe it's good that we have it as a string already=2E
But maybe storing them=
 as TUniqueIds would also be feasible=2E


http://gerrit=2Ecloudera=2Eorg:8=
080/#/c/8621/4/be/src/common/thread-info=2Eh@110
PS4, Line 110:   char quer=
y_id_[TUNIQUE_ID_STRING_SIZE] =3D {};
> The fragment instance ID is actuall=
y the query id plus an index (see Impala
I didn't know that, OK, will get r=
id of it in the next commit=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/86=
21/4/be/src/common/thread-info=2Eh@112
PS4, Line 112:   char thread_name_[T=
HREAD_NAME_SIZE] =3D {};
> 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 th=
at it is a std::string=2E It only contains the thread name on the stack if =
it is small enough and Small String Optimization is used=2E Otherwise, it w=
on't appear in the minidump=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/86=
21/2/be/src/runtime/query-state=2Ecc
File be/src/runtime/query-state=2Ecc:
=

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8621/2/be/src/runtime/query-state=
=2Ecc@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 w=
ou
OK, I agree=2E
I think this parent-relation can work for thread pools al=
so=2E The parent-child hierarchy would represent the task-tree, not the thr=
ead creation tree (which is not that interesting if threads are created in =
a pool)=2E


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

Gerrit-P=
roject: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit=
-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number:=
 8621
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <boroknagyz@cloude=
ra=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Gerrit-Reviewe=
r: Lars Volker <lv@cloudera=2Ecom>
Gerrit-Reviewer: Philip Zeyliger <philip=
@cloudera=2Ecom>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>=

Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera=2Ecom>
Gerrit-Comm=
ent-Date: Tue, 28 Nov 2017 21:31:50 +0000
Gerrit-HasComments: Yes

--mc07qZ37euE=--