impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Borok-Nagy (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Date Wed, 22 Nov 2017 16:14:59 GMT
Zoltan Borok-Nagy 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 1:

(2 comments)

Thanks for the review.
I added SetThreadName(), SetQueryId(), and SetInstanceId() to InfoTable. They have their separate
buffer on the stack.

> "how hard will it be to visit a /threads debug page and see what all the threads are
up to?"
If you have a complete core file (one that is not generated from a minidump), one can traverse
all the threads and inspect the global impala::thread_info_table object and retrieve the needed
information.
If you have a core that is created from a minidump, it is only a little bit more complicated.
During thread traversion one need to select the stack frame that has the InfoTable object
(currently it is Thread::SuperviseThread()). After the needed information can be retrieved
from the InfoTable object.

> "Have you tried pulling one of these out in a minidump?"
I used minidumps, but I converted them to core files because the tooling for minidumps are
not that user friendly / feature rich.

> RegisterAppMemory
Yes, this can be used, but then I would need to call RegisterAppMemory on every thread creation,
and UnregisterAppMemory on every thread destruction. It is not that big a deal, since I can
put this logic into InfoTable's constructor/destructor pairs.
I would also need to make the breakpad ExceptionHandler object accessible for InfoTable, and
synchronize the accesses to it between threads.
With stack allocation, everything is automatically included in the minidump. If you say we
cannot afford having this object on the stack, I rewrite it to use the heap.

> testing
I created backend tests in thread-info-test.cc

http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h
File be/src/common/thread-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@70
PS1, Line 70: /// Call this global function to add information about the current thread
> Should we document that you should avoid information that's sensitive here?
I removed this function, now only the member AddExtraInfo() can be used to add unstructured
text info to the InfoTable.
I added a comment there that it is not for sensitive data.


http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@73
PS1, Line 73: /// is a no-op.
> It's weird to me that this can be a no-op if there's no object, but it fail
I removed this function, now we cannot call InfoTable::AddExtraInfo() without an object.



-- 
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: 1
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 16:14:59 +0000
Gerrit-HasComments: Yes

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