impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2550: Switch to per-query exec rpc
Date Mon, 17 Apr 2017 22:11:06 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2550: Switch to per-query exec rpc
......................................................................


Patch Set 6:

(4 comments)

Looked through some of the files I'm more familiar with - had a few minor comments.

http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/common/status.h
File be/src/common/status.h:

Line 22: #include <ostream>
Can you include iosfwd instead. status.h is included everywhere and it would be nice not to
pull in and compile the huge iostream headers for every file in the codebase.


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 340:   /// only populated by test c'tor
Isn't it also used by the fe-support code path?


http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

Line 23: #include "common/names.h"
common/names.h usually gets put below the other includes because it checks to see what other
headers were pulled in.


Line 61:   if (buffer_pool_min_buffer_len_ != -1 && buffer_pool_capacity_ != -1) {
Thanks for fixing this.


-- 
To view, visit http://gerrit.cloudera.org:8080/6535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message