Henry Robinson has posted comments on this change. Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift ......................................................................
Patch Set 1: (8 comments) 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 c Done 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 proto Done 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. Done 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 The idea is that the c'tors take mandatory parameters. Doing it this way means there's no way to construct a server without a name or processor or port, which means you don't need to check the error of whether they're set or not. I'm not sure what you mean by "options are being set at the caller side vs passing members into a constructor" - can you clarify? PS1, Line 259: auth_provider > Since this is a builder class, I'd prefer if all these functions were prepe I prefer not, unless you feel strongly - the 'set_' is redundant with the context of it being a builder. Line 311: Status Build(ThriftServer** server) { > Thorough error checking needs to be done on the builder side before creatin Any use of the server will trigger a failure if it's misconfigured (and I think duplicating error checking here probably doesn't make sense for that reason). 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 Hm, why? That introduces an extra statement, and is kind of against the point of a fluent interface (where you can chain things together). 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 Not sure this is warranted (you could argue the same thing about 'builder'). It's usually ok for local variables to extend their scope to the end of a method. -- 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: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
