Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: [WIP] Ranger client ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: PS5: > Seems like this conflicts with https://gerrit.cloudera.org/c/15074/4/src/ku We agreed on the proto format offline, I rebased this patch on top of https://gerrit.cloudera.org/c/15074/4/src/kudu/ranger/ranger.proto http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.h@65 PS5, Line 65: std::shared_ptr<kudu::subprocess::SubprocessServer> server_; : }; : : } // namespace ranger : } // namespace kudu > nit: add docs. Done http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.h@71 PS5, Line 71: > Is shared_ptr needed here? Did you consider using unique_ptr and having the I use a mock server in the unit tests so I need to use it outside the class. Do you have a better way to do it in this case? http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_client.h@44 PS3, Line 44: const std::string& table_name) WARN_UNUSED_RESULT; : : // Authorizes action on multiple tables. If there is at least one table that : // user is authorized to performa action on, it returns OK and sets : // 'table_names' to the tables the user is authorized to access. Otherwise it : // returns NotAuthorized and clears 'table_names'. : Status AuthorizeAction(const std::string& user_name, const Action& action, : std::unordered_set<std::string>* table_names) WARN_UNUSED_RESULT; : : // Authorizes an action on multiple columns of a single table. Returns OK if : // 'user_name' is authorized to perform the action on at least one column, : // NotAuthorized otherwise. Sets the column_names vector to the full list of : // tables the user is authorized to access. : Status AuthorizeAction(const std::string& user_name, const Action& action, : const std::string& table_name, std::vector<std::string>* column_names) : WARN_UNUSED_RESULT; : : private: : // Sends request to the subprocess and parses the response. > FWIW I agree with Hao. I was kind of surprised to see so much business logi We discussed it offline, I'm using Hao's proto now. On the other hand the Ranger Client still has some business logic and the Ranger authz provider is the one that is lighter. Let me know what you think about the current rev. http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.cc@60 PS5, Line 60: : auto packed_req = new Any(); : > We probably don't need to plumb this all the way down here. Maybe just leav Done http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.h File src/kudu/subprocess/server.h: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.h@86 PS5, Line 86: virtual ~SubprocessServer(); : : // Initialize the server, starting the subprocess and worker threads. : virtual Status Init() WARN_UNUSED_RESULT; : : // Synchronously send a request to the subprocess and populate 'resp' with : // contents returned from the subprocess, or return an error if anything : // failed or timed out along the way. : virtual Status Execute(SubprocessRequestPB* req, SubprocessResponsePB* resp) WARN_UNUSED_RESULT; > Why these changes? Need to override them in the mock class I use in the unit tests. http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.cc@134 PS5, Line 134: || !process_->IsStarted()) { > When do we shut down before calling Init()? Shutdown is called in the destructor so it will be called even if we fail to Init. I also don't call init in my mock class that I use in the unit tests. -- 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: 6 Gerrit-Owner: Attila Bukor <[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, 17 Feb 2020 07:32:37 +0000 Gerrit-HasComments: Yes
