Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster ...................................................................... Patch Set 9: (17 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322 PS2, Line 322: Configures the HMS to use the Sentry post-event listener, which : // synchronizes the HMS events with t > Ah, I think my confusion is in separating out the rules for the HMS vs the Discussed with Andrew offline. Updated it based on his suggestion. Although note that config is not specific to Kudu instead is for entire HMS operations. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@79 PS8, Line 79: sentry_service_principal > nit: perhaps DCHECK this isn't empty? Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@229 PS8, Line 229: : static const string kHiveFileTemplate = R"( : <con > nit: should be a part of kHiveSentryFileTemplate? Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@310 PS8, Line 310: : // - hive.metastore.filter.hook : // Configures the HMS to use the Sentry plugin for filtering : // out information user has no priv > Does this mean that we'll only see HMS notifications that the `kudu` user h This is only for SHOWTABLES and SHOWDATABASES. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@321 PS8, Line 321: ve.metastore.event.listeners : // Configures the HMS to use the Sentry post-event listener, which : // synchronizes the HMS events with the Sentry service. The Sentry : // service will be made aware of events like table renames and : // update itself accor > nit reword a bit: It is not. That is for HDFS. Reword based on your recommendation, although it is synchronizing the HMS events with the Sentry service not the other way around. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@46 PS8, Line 46: using std::string; > warning: using decl 'ExternalMiniCluster' is unused [misc-unused-using-decl Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@51 PS8, Line 51: // integration enabled. > warning: using decl 'vector' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@95 PS8, Line 95: > nit: kTableIdentifier Done http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], spn, ktpath, > Search for THRIFT_BIND_HOST in HIVE-20794. Does that address HIVE-18998? It Hmm, it looks like so. Although all of our work are based Hive 2.3.x currently. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc@231 PS8, Line 231: 2. Start the HMS, configured to talk to the Sentry service. Find out : // which port it's on > It's been a while since we talked about it, but is it not possible that ano Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc@64 PS8, Line 64: case EXTERNAL_SERVER: : idx = kServersMaxNum / 3 + index; > Somewhat of a nit: this is giving the larger partition to the masters, when Given the kServerMaxNum is 62, that means TSERVER can range from [1,20) I think this is enough for our test cases. But I can switch the calculation between master and tserver. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@114 PS8, Line 114: 127.0.0.1 > Why doesn't this need to be the HMS URIs? This 'hack' works, because we don't use other IP address for the HMS. And I would like to keep it this way as it is simple. And we can always change it if it no longer applies. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@264 PS8, Line 264: kudu,hive > Should this depend on IsHmsEnabled()? I don't think it is necessary, and may bring complexity to the code. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto@65 PS8, Line 65: // Whether or not the Sentry service should be set up. > This isn't sufficient; you also have to update tool_action_test.cc (I think Would you mind I do this in a follow up patch? This reminds me to add a test in kudu-tool-test for HMS administrative tool when running with Sentry integration enabled. http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@402 PS3, Line 402: : // The '-Ffn' flag gets lsof to output something like: : // p5801 : // f548 : // n127.0.0.1:43954->127.0.0.1:43617 : // f549 > Right, but before didn't it expect (n*) within the first three lines? And o It is because we only use this for wildcard binding so far. And with this change, now a daemon (i.e. Sentry) can bind to a specific IP address. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/util/test_util.cc@411 PS8, Line 411: In each pair, the first half of the pair : // is file descriptor number, we ignore it. > nit: wrap the line? Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/util/test_util.cc@436 PS8, Line 436: efix(addr_pa > nit: cur_line? Done -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 12 Dec 2018 22:31:07 +0000 Gerrit-HasComments: Yes
