Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger ...................................................................... Patch Set 6: (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: } > This is already done in each Kudu process (see ExternalDaemon::StartProcess Done 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("\nport = $0\n", port_)); > Why was this change needed? Isn't 127.0.0.1 the default listen address? Wou Done 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 bet in this case this is not as important as in RangerClient, but I still think it makes sense to do it this way for the sake of consistency. I can change it if you feel strongly about it though. 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: > Nit: static (and then non-static) private member functions should be above Done 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 KRB5_CONFIG is a standard env var used to set the location of krb5.conf if it's not the default location. Unfortunately Java doesn't respect this, and as this can also be problematic in a real environment, not only in MiniCluster, I thought it would be best to set it based on this env var instead of manually configuring it. This way the master and the subprocess are guaranteed to use the same krb5.conf (there still can be some discrepancies in behavior though, as Java doesn't support every option, but there's nothing we can do about that). http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@198 PS4, Line 198: } > This was for debugging I presume, remove now? Done -- 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: 6 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 21:34:44 +0000 Gerrit-HasComments: Yes
