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 Thu, 23 Nov 2017 12:26:27 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 4:

(13 comments)

Thanks all!

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:     info_table.AddExtraInfo("extra", "info");
> I agree it would be helpful to have some more comments. Maybe at least a co
Done


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info-test.cc@55
PS2, Line 55:     info_table.AddExtraInfo("extra", "info");
> please add a comment about what's going on:
Done


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: /// function 'GetThreadInfoTable()'.
> "Until this object lives" -> "While this object is alive"
Done


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@41
PS2, Line 41: public:
> This is the only usage of boost:noncopyable I've seen in our code base. We 
Done


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@41
PS2, Line 41: public:
> Yeah DISALLOW_COPY_AND_ASSIGN is the canonical way to do it in Impala (for 
Done


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@42
PS2, Line 42:   // Only one InfoTable object can be alive per thread at a time.
> We've generally been moving to using the new inline initialisation syntax f
Done


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@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.
I added a comment about it.


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@61
PS2, Line 61:     }
> Would line.copy(text + text_size, line.length()) be clearer here?
The buffers were initialized to null by the constructor's init-list. Now they are initialized
to null by the inline member initializers.

I switched to the member copy.


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@101
PS2, Line 101: dInfo(
> We generally use int64_t for byte sizes in the codebase. There are argument
Done


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


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@108
PS2, Line 108:   int64_t text_size_ = 0;
> You're choosing to store this as a string intentionally?
I thought this makes easier to display it during a debug session.
Also, if we want to use it for prepending log lines, it might be more efficient to have it
as a string already.
But I can switch to TUniqueId if needed.


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/common/thread-info.h@115
PS2, Line 115: };
> By convention we generally use pointers if arguments or return values are m
Done


http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/8621/2/be/src/runtime/query-state.cc@378
PS2, Line 378:   thread_info_table->SetQueryId(fis->query_id());
> We should also do this for other threads running in the context of the quer
For starter, I fill the InfoTable on these callsites.

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

What do you think about it?



-- 
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: 4
Gerrit-Owner: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@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: Thu, 23 Nov 2017 12:26:27 +0000
Gerrit-HasComments: Yes

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