impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Borok-Nagy (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables
Date Thu, 14 Dec 2017 13:42:43 GMT
Zoltan Borok-Nagy has posted comments on this change. (

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

Patch Set 10:


Thank you very much!
Commit Message:
PS10, Line 11: in debug sessions. It needs to be allocated on the stack in
> mention "on the stack of each thread"
PS10, Line 22: fully-fledged core
> Replace with "core dump written by Impala"?
File be/src/common/
PS10, Line 20: #include <vector>
> Still needed?
No, removed it and others as well.
PS10, Line 30: using boost::split;
> still needed?
No, removed it.
File be/src/common/thread-debug-info.h:
PS10, Line 83:   void PrintUniqueIdToMember(const TUniqueId& id, char* member) {
> We could inline this now as it's only used once.
PS10, Line 90:   static void InitializeThreadDebugInfo(ThreadDebugInfo* thread_debug_info);
> I think these could benefit from a brief comment since they're defined in a
Added short comments about them.
PS10, Line 96:   static constexpr int64_t THREAD_NAME_FRONT_LENGTH = THREAD_NAME_SIZE -
> I think moving this computation to SetThreadName() would make it slightly e
Should I remove THREAD_NAME_TAIL_LENGTH as well?
File be/src/exec/
PS10, Line 199:           ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
> You could also do GetThreadDebugInfo()->SetInstanceId(state->fragment_insta
yes, previously I set more member (query id).

File be/src/exec/
PS10, Line 353:       ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
> You could shorten it a bit to GetThreadDebugInfo()->SetInstanceId(state->fr
File be/src/runtime/
PS10, Line 232:           ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
              :           thread_debug_info->SetInstanceId(this->instance_id());
> You could shorten this to GetThreadDebugInfo()->SetInstanceId(this->instanc
File be/src/runtime/
PS10, Line 377: thread_debug_info
> Inline the call to GetThreadDebugInfo() here, too?

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: 10
Gerrit-Owner: Zoltan Borok-Nagy <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Thu, 14 Dec 2017 13:42:43 +0000
Gerrit-HasComments: Yes

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