impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5167: Reduce the number of Kudu clients created
Date Tue, 09 May 2017 18:28:12 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5167: Reduce the number of Kudu clients created
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6792/1//COMMIT_MSG
Commit Message:

Line 22: 
> Turns out we already have a test that covers this -
did your manual testing involve concurrency? I think it's still an area for tests


http://gerrit.cloudera.org:8080/#/c/6792/2//COMMIT_MSG
Commit Message:

PS2, Line 20: fdc022fe6231af20e307012d98c35b16cbfa7b33
While this change doesn't have a compile time dependency on the client, this does need these
changes to come into our C++ client via the toolchain.

(...and I'm having issues with my toolchain update.)


http://gerrit.cloudera.org:8080/#/c/6792/2/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

Line 80:       client_(nullptr),
move to the header


PS2, Line 83:       total_rows_(NULL),
            :       num_row_errors_(NULL),
            :       rows_processed_rate_(NULL)
can you set these to nullptr as well in the header

replace all NULLs will nullptrs in file


Line 145:   Status s =
fit on 1line?


http://gerrit.cloudera.org:8080/#/c/6792/2/be/src/exec/kudu-table-sink.h
File be/src/exec/kudu-table-sink.h:

PS2, Line 103: client_
set to nullptr here


http://gerrit.cloudera.org:8080/#/c/6792/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS2, Line 403: master_addresses
can you sort the list before joining? That way we can avoid dupes between serverA,serverB
 and serverB,serverA


http://gerrit.cloudera.org:8080/#/c/6792/2/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

PS2, Line 193: fragment instance
this is wrong now


PS2, Line 193: Each Kudu table has
             :   /// a list of master addresses stored in the Hive Metastore. 
I don't think this sentence is relevant. Remove


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b0c12a256c33e8ef32315b3736cae2dea2ae705
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message