Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15018 )
Change subject: KUDU-2973 Normalize table names for Ranger ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/15018/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15018/2//COMMIT_MSG@7 PS2, Line 7: KUDU-2973 Normalize table names for Ranger > Updated the description for KUDU-2973 to be more clear. It doesn't fully match what's in the JIRA, I explained why in the updated commit message and in https://gerrit.cloudera.org/c/15018/2/src/kudu/master/catalog_manager.cc#5168 I'm not completely sure about the "default" database though. I could add a separate ParseRangerTableIdentifier() method that returns "default" db name for Ranger and this wouldn't get synced to HMS, or I could modify ParseHiveTableIdentifier() to default the db name to "default" for Hive and for internal use only (this would mean we change foobar to default.foobar everywhere though), or I could just skip Ranger authz for tables with no database names. Now that I wrote this down I think maybe the Ranger-only "default" database name makes most sense as these tables would never get synced to HMS anyway so consistency shouldn't be an issue. This would mean Ranger would authorize requests on all tables, but HMS sync would remain the same - only tables in format "database.table" would be synced to HMS. What do you think? http://gerrit.cloudera.org:8080/#/c/15018/2//COMMIT_MSG@12 PS2, Line 12: insensiti > You mean insensitive right? Done http://gerrit.cloudera.org:8080/#/c/15018/2/src/kudu/master/CMakeLists.txt File src/kudu/master/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15018/2/src/kudu/master/CMakeLists.txt@65 PS2, Line 65: kudu_sentry > Nit: out of order. Done http://gerrit.cloudera.org:8080/#/c/15018/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15018/2/src/kudu/master/catalog_manager.cc@5168 PS2, Line 5168: if (hms::HmsCatalog::IsEnabled() || > Will this patch includes database name parsing (based on the table name)? I think we can reuse `ParseHiveTableIdentifier` from `table_util`. Should I rename it to something non-Hive? I think the table name should be case insensitive in Ranger as well after all, because if a user turns on authz with Ranger first, grants some permissions on them, then turns on HMS sync, they need to be consistent without having to change the permissions. Impala, Hive, Spark etc using HMS would authorize requests before sending them to Kudu, so if the db/table names in HMS don't match the ones in Ranger the requests would be denied before Kudu had a chance to properly authorize them. http://gerrit.cloudera.org:8080/#/c/15018/2/src/kudu/ranger/CMakeLists.txt File src/kudu/ranger/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15018/2/src/kudu/ranger/CMakeLists.txt@25 PS2, Line 25: gflags) > Why this dependency? I don't see any includes in the new code that pulls in Done -- To view, visit http://gerrit.cloudera.org:8080/15018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 Gerrit-Change-Number: 15018 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Anonymous Coward (314) Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 14 Jan 2020 13:02:33 +0000 Gerrit-HasComments: Yes
