impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class
Date Fri, 01 Sep 2017 19:43:32 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message