impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Date Thu, 03 Aug 2017 20:52:13 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message