Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift ......................................................................
Patch Set 1: (12 comments) A lot of the comments are just about refactoring. Feel free to disagree with them if you think it's better the way it is. http://gerrit.cloudera.org:8080/#/c/7524/1//COMMIT_MSG Commit Message: PS1, Line 11: allows nit: allow http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server-test.cc File be/src/rpc/thrift-server-test.cc: Line 276: I would add one more test case where the server is started with a list of ciphers and the client is started with a different list of ciphers such that there is an overlap of a few ciphers; and test that things work as expected. Eg: Server has 3DES and RSA Client has AES256 and RSA Things should work because RSA is common between them? (If the above is a valid example) http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: PS1, Line 166: friend class ThriftServerBuilder; nit: Move just below private: as it's easier to find, plus that's the protocol usually followed to declare a friend. PS1, Line 247: // Returns true if, per the process configuration flags, server<->server communications : // should use SSL. : bool EnableInternalSslConnections(); nit: Move below ThriftServerBuilder class or above ThriftServer class. Good to have the builder and target class next to each other for readability. PS1, Line 254: ThriftServerBuilder(const std::string& name, : const boost::shared_ptr<apache::thrift::TProcessor>& processor, int port) : : name_(name), processor_(processor), port_(port) {} Builder constructors usually just take the name from what I've seen in the past. Everything else would ideally be set as an option. Plus, that looks easier to read by knowing what options are being set at the caller side vs. passing members into a constructor. The Build() function can return an error if the required parameters are not set. PS1, Line 259: auth_provider Since this is a builder class, I'd prefer if all these functions were prepended with "set_" Eg: set_auth_provider(), etc. Line 311: Status Build(ThriftServer** server) { Thorough error checking needs to be done on the builder side before creating the target class object. Eg: if (port_ == 0) This is if you decide to change the constructor as I suggested above. http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS1, Line 1947: be_builder.metrics(exec_env->metrics()) Set metrics just after constructor initialization in L1940 and just call be_builder.Build() here. PS1, Line 1974: builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider()) : .metrics(exec_env->metrics()) : .thread_pool(FLAGS_fe_service_threads) Same here, set all options just after constructor init and just call builder.Build() here. PS1, Line 2000: builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider()) : .metrics(exec_env->metrics()) : .thread_pool(FLAGS_fe_service_threads) : .Build(hs2_server)); Same as above http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: PS1, Line 203: ThriftServer* server; : RETURN_IF_ERROR(builder.Build(&server)); : heartbeat_server_.reset(server); Surround with curly braces so that 'server' goes out of scope when it's not needed? http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestored-main.cc File be/src/statestore/statestored-main.cc: PS1, Line 89: metrics(metrics.get()) move to L83. Also follow this pattern everywhere else if you agree. -- To view, visit http://gerrit.cloudera.org:8080/7524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
