Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/mini-cluster/external_mini_cluster.cc@647 PS4, Line 647: opts.extra_flags.emplace_back("--unlock_experimental_flags"); This is already done in each Kudu process (see ExternalDaemon::StartProcess). http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/postgres/mini_postgres.cc File src/kudu/postgres/mini_postgres.cc: http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/postgres/mini_postgres.cc@135 PS4, Line 135: config.append(Substitute("\nlisten_addresses = '127.0.0.1'\nport = $0\n", port_)); Why was this change needed? Isn't 127.0.0.1 the default listen address? Would be nice to doc with a comment. http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206 PS4, Line 206: string krb5_config = getenv("KRB5_CONFIG"); Again, this is an opaque way to pass Kerberos configuration information between cluster daemons. I'd prefer if you did what we did for Sentry/HMS: an EnableKerberos method that takes all the (unmarshalled) information it needs as separate arguments. Then, ExternalMiniCluster can call that method directly and feed in information from the MiniKdc's environment variables. http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.h@117 PS4, Line 117: static std::string java_path(); Nit: static (and then non-static) private member functions should be above the data members. http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195 PS4, Line 195: string krb5_config = getenv("KRB5_CONFIG"); If this is the Kerberization switch, would rather it be an explicit setter called on RangerClient, because otherwise it's tough to trace the relationship between whomever consumes KRB5_CONFIG (RangerClient) and whomever sets it (the EMC, presumably). http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@198 PS4, Line 198: LOG(INFO) << krb5_config; This was for debugging I presume, remove now? -- To view, visit http://gerrit.cloudera.org:8080/15601 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549 Gerrit-Change-Number: 15601 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 31 Mar 2020 20:35:49 +0000 Gerrit-HasComments: Yes
