Todd Lipcon has posted comments on this change. Change subject: [TLS cert management] security service implementation ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/5674/1/src/kudu/security/security_service.cc File src/kudu/security/security_service.cc: PS1, Line 61: (s) this macro evaluates 's' a bunch of times, which is no good if it has side effects. The usual style we do is something like: const Status& _s = (s); ... and then use _s PS1, Line 77: ((s).ToString() this is redundant with the string which already ends up in the statuspb (set by StatusToPB above) PS1, Line 86: ConvertFromPBFormat usual style is 'DataFormatFromPB' Line 88: if (PREDICT_FALSE(!format)) { just CHECK or DCHECK PS1, Line 111: server->result_tracker() hm, given that this .cc depends on kudu/rpc, I think we probably need it to be a separate cmake module. That or just put it directly into src/kudu/master/ since our only usage of it will be within the master Line 136: unique_ptr<string> ca_cert_str(new string); this is a bit strange - I think a more readable way with the same perf would be: string ca_cert_str; ... ca_cert_pb->mutable_data()->swap(ca_cert_str); or *ca_cert_pb->mutable_data() = std::move(ca_cert_str); Line 167: unique_ptr<string> cert_str(new string); see above http://gerrit.cloudera.org:8080/#/c/5674/1/src/kudu/security/security_service.h File src/kudu/security/security_service.h: Line 1: // Licensed to the Apache Software Foundation (ASF) under one think it makes sense to name this file specifically for the cert-signing service (do we anticipate multiple new services?) PS1, Line 42: master::Master this cyclic reference is a no-no. -- To view, visit http://gerrit.cloudera.org:8080/5674 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
