impala-reviews mailing list archives

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

Mime
View raw message