Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 13: (18 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 ParseRanger Done 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: > Add a doc here? Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h@29 PS12, Line 29: : : : : : > Maybe I forgot that for Kudu's Ranger policy, there is no such entity scopi Done 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: column_name == rhs.column_name; > This seems like it could be stack allocated. Partially done, but I think I'm missing something. How would I inject the mock subprocess server to the real client if it's stack allocated in RangerClient? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@124 PS12, Line 124: auto packed_resp = new Any(); : packed_resp->PackFrom(resp_list); : resp->set_allocated_response(packed_resp); : : r > Rewrite as AddResponse(table == "default.foobar"); Done 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: // > Why do these functions need to be virtual? Is it for future mocking? If so, Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@41 PS12, Line 41: > You can drop the kudu prefix and I think get by with one of the following: Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@54 PS12, Line 54: > perform an Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@56 PS12, Line 56: perform an action on, it > Why bother? Won't the NotAuthorized alone signal to the caller that the use Done 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: return server_->Init(); > Doesn't seem all that interesting; maybe omit it? Or downgrade to VLOG? Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@67 PS12, Line 67: > We should handle the return value in some way. added a DCHECK, do you think that's enough? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@75 PS12, Line 75: Slice tbl; : : // ParseRangerTableIdentifier > Let's move this down to where we actually need it (i.e. L86). Done 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 statu Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@94 PS12, Line 94: req->set_table(tbl.ToString()); > Use DCHECK_EQ. Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@136 PS12, Line 136: req->set_column(col); > To adhere to the common calling convention used in Kudu, let's restructure Done http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@170 PS12, Line 170: > Nit: why this empty line? Done 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: // > I'd supplement this doc with a quick note about why the public methods are Done 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()) { > This isn't super robust because it assumes a particular ordering in Init(); Done -- 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: 13 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: Mon, 02 Mar 2020 18:31:59 +0000 Gerrit-HasComments: Yes
