Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 22: (12 comments) http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/mini-cluster/external_mini_cluster.cc@337 PS16, Line 337: if (opts_.enable_ranger) { > I was planning to do that in a follow-up, we need to configure the subproce No, follow-up is fine. Just wanted clarification. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger-test.cc File src/kudu/ranger/mini_ranger-test.cc: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger-test.cc@75 PS22, Line 75: ASSERT_TRUE(s.IsRemoteError()); Anything we can check in the error message to prove to ourselves that "policy already found" was the result? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h@72 PS22, Line 72: private: Please doc all of the non-accessor methods. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@40 PS16, Line 40: typedef std::pair<std::vector<std::string>, std::vector<ActionPB>> PolicyItem; > Done No doc? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@91 PS22, Line 91: We're not using this shutdown port as we simply send a SIGTERM, : // but it's necessary to set it to a random value to avoid collisions in : // parallel testing. Not possible to tell Ranger not to bind to it at all? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@131 PS22, Line 131: Env* env = Env::Default(); This seems to be repeated a bunch; how about create a single env_ at MiniRanger constructor time? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@150 PS22, Line 150: // TODO(abukor): load a db dump instead as this is very slow How slow? Anything we can do on the postgres side (some reconfiguration perhaps) to speed it up? For example, we don't need durability in these tests at all, so if we can tell postgres not to fsync or whatever, we may be able to speed things up. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@316 PS22, Line 316: EasyCurl curl; : faststring result; : curl.set_auth(CurlAuthType::BASIC, "admin", "admin"); : RETURN_NOT_OK(curl.PostToURL(JoinPathSegments(ranger_admin_url_, : "service/plugins/policies"), : policy_json.ToString(), &result, : {"Content-Type: application/json"})); CreateKuduService shares a lot of this. Decompose into a (parameterized) helper? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.h File src/kudu/util/curl_util.h: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.h@61 PS22, Line 61: Status PostToURL(const std::string& url, Doc 'headers'. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.h@143 PS22, Line 143: std::string username_ = ""; : : std::string password_ = ""; Don't need the initializers. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.cc@125 PS22, Line 125: CURL_RETURN_NOT_OK(curl_easy_setopt(curl_, CURLOPT_HTTPAUTH, CURLAUTH_ANY)); Set this in the default case of the switch below. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.cc@126 PS22, Line 126: switch (auth_type_) { Should add a default so that the compiler doesn't warn. -- 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: 22 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: Sun, 22 Mar 2020 22:19:47 +0000 Gerrit-HasComments: Yes
