Todd Lipcon has posted comments on this change. Change subject: [TLS cert management] security service units tests ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/5675/1/src/kudu/security/test/security_service-test.cc File src/kudu/security/test/security_service-test.cc: Line 1: // Licensed to the Apache Software Foundation (ASF) under one meta-note: generally not a fan of splitting patches into one for the code and one for the tests. I usually want to look at the tests as I review the code to understand how it's used, etc. Line 81: mini_master_.reset(new MiniMaster(Env::Default(), similar to comment in parent patch -- we don't want this upward dependency from security/ to master/, because now we have an indirect dependency of stuff like the rpc/ system all the way down to the master, which will be a big pain for folks like Impala who are importing our RPC. Line 120: const char SecurityServiceTest::kCaCert_[] = R"***( can we centralize these in security/test/test_certs.{h,cc}? Line 218: for (size_t i = 0; i < arraysize(kFormats); ++i) { can use a normal foreach? -- To view, visit http://gerrit.cloudera.org:8080/5675 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9 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
