Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 12: (17 comments) http://gerrit.cloudera.org:8080/#/c/15206/12//COMMIT_MSG Commit Message: PS12: Could you comment in the commit message regarding the change to ParseRangerTableIdentifier? You can also talk about the level of test coverage in this patch vs. what's to come. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h File src/kudu/ranger/ranger_action.h: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h@27 PS12, Line 27: enum class Action { Add a doc here? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@55 PS12, Line 55: shared_ptr<RangerResponseListPB> next_response_; This seems like it could be stack allocated. You could also make it a field of RangerClientTest and pass a pointer to it into the mocked subprocess server. That way AddResponse needn't access the server in order to add a response, allowing you to make the server wholly owned by the RangerClient (and then adopting one of Andrew's suggestions regarding unique_ptr or stack allocation). http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@124 PS12, Line 124: if (table == "default.foobar") { : AddResponse(true); : } else { : AddResponse(false); : } Rewrite as AddResponse(table == "default.foobar"); Below too. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@38 PS12, Line 38: class RangerClient { Why do these functions need to be virtual? Is it for future mocking? If so, could you doc that? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@41 PS12, Line 41: kudu You can drop the kudu prefix and I think get by with one of the following: ::subprocess::SubprocessServer ::SubprocessServer http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@54 PS12, Line 54: performa perform an http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@56 PS12, Line 56: and clears 'table_names'. Why bother? Won't the NotAuthorized alone signal to the caller that the user has no access to any of the tables? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@54 PS12, Line 54: LOG(INFO) << "Initializing Ranger subprocess server"; Doesn't seem all that interesting; maybe omit it? Or downgrade to VLOG? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@67 PS12, Line 67: sresp.response().UnpackTo(resp); We should handle the return value in some way. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@75 PS12, Line 75: RangerRequestListPB req_list; : RangerResponseListPB resp_list; : req_list.set_user(user_name); Let's move this down to where we actually need it (i.e. L86). Same below. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@82 PS12, Line 82: if (PREDICT_FALSE(!s.ok())) { : return Status::OK(); : } At the very least this warrants a comment: why should we ignore a bad status here and return "you are authorized"? Same below. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@94 PS12, Line 94: DCHECK(resp_list.responses_size() == 1); Use DCHECK_EQ. Same below. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@136 PS12, Line 136: column_names->clear(); To adhere to the common calling convention used in Kudu, let's restructure the function so that we don't modify any user input in the event of a bad Status. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@170 PS12, Line 170: Nit: why this empty line? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.h File src/kudu/subprocess/server.h: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.h@187 PS12, Line 187: class SubprocessServer { I'd supplement this doc with a quick note about why the public methods are virtual (i.e. so that you can more easily mock the server). http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.cc@141 PS12, Line 141: if (!closing_.CountDown() || !process_->IsStarted()) { This isn't super robust because it assumes a particular ordering in Init(); if that order changes, suddenly we will have forgotten to clean something up. Instead, I'd suggest adding conditions to the individual cleanup operations in Shutdown. If the process was started, call KillAndWait. If a thread exists, join on it. And so on. -- To view, visit http://gerrit.cloudera.org:8080/15206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 Gerrit-Change-Number: 15206 Gerrit-PatchSet: 12 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: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 26 Feb 2020 21:24:39 +0000 Gerrit-HasComments: Yes
