Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 26: (10 comments) http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.h@53 PS26, Line 53: on a resource Is this the policy for a single resource? The vectors of databases, tables, and columns still isn't super clear. Are the privileges granted to the cross-product? E.g. If I have {db1, db2}->{table1, table2}->{column}, does that mean - db1->table1->column - db1->table2->column - db2->table1->column - db2->table2->column ? http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.h@54 PS26, Line 54: user nit: users http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.h@76 PS26, Line 76: curl_.set_auth(CurlAuthType::BASIC, "admin", "admin"); : } nit: remove a few spaces http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.h@126 PS26, Line 126: $$CLASSPATH Will this be evaluated too? http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.cc@238 PS26, Line 238: Faled nit: Failed http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.cc@235 PS26, Line 235: Status s = POSTToRanger("service/plugins/services", std::move(service), &result); : if (s.IsRemoteError()) { : return Status::RemoteError( : Substitute("Faled to create Kudu service. Status: $0; message: $1", : s.ToString(), result.ToString())); : } : return s; Could probably just RETURN_NOT_OK_PREPEND(POSTToRanger(...), Substitute("failed to create Kudu service: {result})); return Status::OK(); http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.cc@303 PS26, Line 303: POSTToRanger nit: keep the casing consistent? PostToRanger? http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger.cc@303 PS26, Line 303: string nit: const string& http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger_configs.h File src/kudu/ranger/mini_ranger_configs.h: http://gerrit.cloudera.org:8080/#/c/15483/26/src/kudu/ranger/mini_ranger_configs.h@64 PS26, Line 64: RPC nit: RPCs http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/tools/tool.proto@45 PS22, Line 45: // Whether or not Ranger should be enabled : optional bool enable_ranger = 9; : > not sure why Sentry doesn't have one. I did this so I can test the REST API Ah I see. I think virtually all of our usage of Sentry happened through unit tests. I don't mind keeping it in. -- To view, visit http://gerrit.cloudera.org:8080/15483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15ab1eb8abe71c074c26b286073442882e101bc6 Gerrit-Change-Number: 15483 Gerrit-PatchSet: 26 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Mon, 23 Mar 2020 08:29:21 +0000 Gerrit-HasComments: Yes
