Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15394 )
Change subject: WIP - KUDU-3050: recover from corrupt kerberos ccache ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: PS3: Maybe integration-tests/security-itest would be a better home for this new test? http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@20 PS3, Line 20: #include <errno.h> Prefer cerrno. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@296 PS3, Line 296: ASSERT_EQ(0, truncate(cc_path, 10)) << errno << " " << cc_path; I think it'd be more elegant (albeit more lines of code) to open cc_path as an RWFile (see env.h) and call Truncate on it. You'll get a Status you can ASSERT_OK() on. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@298 PS3, Line 298: // Sleep long enough to ensure that the tserver's ticket expires so that renewal is : // needed. : SleepFor(MonoDelta::FromSeconds(16)); Could we lower the ticket lifetime so that this sleep could be shorter? Alternatively, is there some way we could "sleep until ticket expiration"? http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@306 PS3, Line 306: cluster.master(0)->Shutdown(); With just one master in the cluster, you can use the simpler master() accessor. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@310 PS3, Line 310: cluster.Shutdown(); Don't need to do this; it'll happen in the destructor. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/init.cc@98 PS3, Line 98: RWMutex g_kerberos_reinit_lock(RWMutex::Priority::PREFER_WRITING); Global non-POD fields are generally a bad idea because it's tough to guarantee exactly when the constructors/destructors run. That's why the existing code used a pointer here. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/init.cc@160 PS3, Line 160: << "Renew thread sleeping after " << failure_retries << " failures for " : << renew_interval_s << "s"; Nit: we generally use strings::Substitute() for constructing messages like these; it's easier to see the full message with substitution variables than with interspersed '<<'. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/init.cc@220 PS3, Line 220: krb5_get_error_message Isn't this a memory leak? Use Krb5CallToStatus to handle this safely. Looking at your test results, seems like LSAN caught this. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/test/mini_kdc.cc File src/kudu/security/test/mini_kdc.cc: http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/test/mini_kdc.cc@301 PS3, Line 301: kt_path = JoinPathSegments(options_.data_root, kt_path) + ".keytab"; : return kt_path; Nit: could combine. -- To view, visit http://gerrit.cloudera.org:8080/15394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d6e06c3ea65708896a6bf0134cc84838b3f1b58 Gerrit-Change-Number: 15394 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 10 Mar 2020 06:42:19 +0000 Gerrit-HasComments: Yes
