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 <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
