Michael Ho has posted comments on this change. Change subject: IMPALA-4670: Introduces RpcMgr class ......................................................................
Patch Set 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/7901/3/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: PS3, Line 62: int32_t payload_[PAYLOAD_SIZE]; > 'payload_' can be a private variable. Done PS3, Line 60: TNetworkAddress krpc_address_; : RpcMgr rpc_mgr_; : int32_t payload_[PAYLOAD_SIZE]; > Member variable declarations should go below the functions definitions? Judging from various headers, we aren't quite consistent about it. I have seen examples of both. PS3, Line 75: SetupScanMemRequest > Could you add a brief function header? Done PS3, Line 120: int32_t > nit: I think it would be better to either use int32_t or uint32_t consisten Done PS3, Line 127: > nit: add some delimiter here Done PS3, Line 141: Start a second service which verifies the RPC payload is not corrupted. > nit: Test that a second service, that verifies the RPC payload is not corru Done PS3, Line 188: boost::bind > I'd prefer if we did this with a named lambda instead. What do you think? Done. I think lambda is fine for small callback in test like this but I find it rather hard to read for bigger callback used in DSS. PS3, Line 237: boost::bind > Same here Done http://gerrit.cloudera.org:8080/#/c/7901/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: Line 95: /// port is configurable via FLAGS_acceptor_threads. > I think it would be good to add an example of the function calls a applicat Added pointer to rpc-mgr-test.cc for examples. PS3, Line 129: The RPC layer will accept incoming : /// connections but no service will be available. > Is that true? UnregisterServices() calls Messenger::Shutdown(), which calls Renamed function to Shutdown() and call it in ExecEnv d'tor. PS3, Line 171: } > nit: We don't seem to consistently enforce this rule in many files. May be one should write a script to fix all of these places up. http://gerrit.cloudera.org:8080/#/c/7901/3/be/src/rpc/rpc-mgr.inline.h File be/src/rpc/rpc-mgr.inline.h: PS3, Line 43: } > nit: Done http://gerrit.cloudera.org:8080/#/c/7901/3/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: PS3, Line 32: namespace kudu { : namespace client { : class KuduClient; : } : } > nit: Done http://gerrit.cloudera.org:8080/#/c/7901/3/be/src/util/network-util.cc File be/src/util/network-util.cc: PS3, Line 226: } > Not your change but: Done http://gerrit.cloudera.org:8080/#/c/7901/3/common/protobuf/rpc_test.proto File common/protobuf/rpc_test.proto: Line 43 > Is it not possible to use the existing Kudu protobuf structures for testing Seems to be more flexible to keep it in a test specific file in case we want to experiment with certain cases. -- To view, visit http://gerrit.cloudera.org:8080/7901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
