impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
Date Wed, 25 Jan 2017 20:38:32 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 3:

(6 comments)

some preliminary comments. i'll do a thorough pass in a bit.

http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

Line 18: #ifndef IMPALA_RPC_RPC_MGR_H
> Will postpone for now. There's a dependency on security code which currentl
what do you mean by 'hasn't been integrated yet'?


Line 183
> Done
?


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc_test.proto
File be/src/rpc/rpc_test.proto:

Line 18: package impala;
add comment describing what this is used for


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

Line 76: 
> rpc::StatestoreSubscriberIf comes from statestore.service.h, which is auto-
this could actually help with readability: adding a nested namespace for generated code. so
something like generated::bla.

thoughts?


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

Line 77: class StatestoreSubscriberImpl : public StatestoreSubscriberIf {
calling it 'impl' isn't super-meaningful (impl of what?). would be nice to come up with a
standard name for the service shims. ServiceImpl?


Line 174:   auto rpc = Rpc<StatestoreServiceProxy>::Make(statestore_address_, rpc_mgr_);
definitely an improvement.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message