Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 23: (11 comments) 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: const std::string kExpectedError = "Another policy already exists for matching resource"; > Anything we can check in the error message to prove to ourselves that "poli Done 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: } > Please doc all of the non-accessor methods. Done 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: > No doc? sorry, missed the part about the 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: necessary to set it to a random value to avoid collisions in : // parallel testing. : RETURN_NOT_OK(GetRando > Not possible to tell Ranger not to bind to it at all? Doesn't seem so based on the code. If we just remove that part it will bind to the default port (6185). http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@131 PS22, Line 131: web_app_dir.c_str())); > This seems to be repeated a bunch; how about create a single env_ at MiniRa Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@150 PS22, Line 150: db_setup.SetEnvVars({ > How slow? Anything we can do on the postgres side (some reconfiguration per Around 8 seconds. I don't think Postgres is the bottleneck here though. Also, the Ranger startup itself (after db is set up) is another ~9 seconds and I have no idea if there's anything we can do about it. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@316 PS22, Line 316: if (s.IsRemoteError()) { : return Status::RemoteError(Substitute("Failed to add policy. Status: $0; message: $1", : s.ToString(), result.ToString())); : } : : return s; : } > CreateKuduService shares a lot of this. Decompose into a (parameterized) he Done 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: // The optional param 'headers' holds additional headers. > Doc 'headers'. Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.h@143 PS22, Line 143: char errbuf_[kErrBufSize]; : : std::string username_; > Don't need the initializers. Done 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: switch (auth_type_) { > Set this in the default case of the switch below. Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/util/curl_util.cc@126 PS22, Line 126: case CurlAuthType::SPNEGO: > Should add a default so that the compiler doesn't warn. Done -- 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: 23 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 23:39:43 +0000 Gerrit-HasComments: Yes
