Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15394 )
Change subject: KUDU-3050: recover from corrupt kerberos ccache ...................................................................... Patch Set 4: (10 comments) Thanks for the feedback, I reworked a lot of this in response to comments. I also found and fixed some issues with the recovery logic - I didn't consider the case where it open the file OK but then didn't find the right credential. 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 That's a good suggestion. Moved it there and rewrote it, it got a lot simpler and faster. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@20 PS3, Line 20: #include <iosfwd> > Prefer cerrno. Done http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@296 PS3, Line 296: opts.master_rpc_addresses = master_rpc_addresses; > I think it'd be more elegant (albeit more lines of code) to open cc_path as Done http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@298 PS3, Line 298: NO_FATALS(SmokeExternalMiniCluster(opts, cluster.get(), &master_rpc_addresses)); : ASSERT_EQ(opts.master_rpc_addresses, master_rpc_addresses); : > Could we lower the ticket lifetime so that this sleep could be shorter? After the move of the test to an integration test this is no longer required. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@306 PS3, Line 306: cluster->Shutdown(); > With just one master in the cluster, you can use the simpler master() acces Done http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@310 PS3, Line 310: } // namespace kudu > Don't need to do this; it'll happen in the destructor. Done 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; > Global non-POD fields are generally a bad idea because it's tough to guaran I ran into the problem that the test doesn't call InitKerberosForServer() but does call methods that use the lock. I added this to InitKrb5Ctx(). I could also expose a different method to init the lock, but that seemed ugly. http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/init.cc@160 PS3, Line 160: retries > 0) { : // Log in the abnormal case where som > Nit: we generally use strings::Substitute() for constructing messages like Done http://gerrit.cloudera.org:8080/#/c/15394/3/src/kudu/security/init.cc@220 PS3, Line 220: ibution<> dist(0.5, 1. > Isn't this a memory leak? Use Krb5CallToStatus to handle this safely. Done 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: return JoinPathSegments(options_.data_root, kt_path) + ".keytab"; : } > Nit: could combine. Done -- 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: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 10 Mar 2020 21:21:01 +0000 Gerrit-HasComments: Yes
