Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15018 )
Change subject: KUDU-2973 Add semi-database support for Ranger ...................................................................... Patch Set 11: (11 comments) http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc File src/kudu/common/table_util-test.cc: http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc@65 PS10, Line 65: TEST(TestTableUtil, TestRangerTableIdentifier) { > Some of the lines look like they're too long. Done http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc@77 PS10, Line 77: my_awesome/table/22 > What if the database name is something containing the '/' symbol? Is it a yep, added a test case. http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc@89 PS10, Line 89: "foo" > Does it make sense to add a test for parsing 'default.foo'? Is it recogniz good idea, added. It's not recognized as a default database in this case. http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc@103 PS10, Line 103: EXPECT_TRUE(ParseRangerTableIdentifier(".", &db, &tbl, &default_database) > It think it would be nice to add a test for 'db_name..table_name' as well. added it in the above section as I think it's worth checking correctness as well in this case. http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc@105 PS10, Line 105: EXPECT_OK(ParseRangerTableIdentifier("no_table", &db, &tbl, > It seems line 105 can be removed since there is also line 107 Done http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util-test.cc@110 PS10, Line 110: &default_data > what about unicode characters in the table, not database names? Done http://gerrit.cloudera.org:8080/#/c/15018/9/src/kudu/common/table_util.h File src/kudu/common/table_util.h: http://gerrit.cloudera.org:8080/#/c/15018/9/src/kudu/common/table_util.h@40 PS9, Line 40: // Parses a Kudu table name of the form '<database>.<table>' into a > nit: Can you add a comment for this method? Done http://gerrit.cloudera.org:8080/#/c/15018/9/src/kudu/common/table_util.cc File src/kudu/common/table_util.cc: http://gerrit.cloudera.org:8080/#/c/15018/9/src/kudu/common/table_util.cc@30 PS9, Line 30: Name of the default database which is used in the Ranger " : "authorization context when the database name is not specified " : "in the table name. > Should we note here that if users should be careful about creating tables w Documented it, leaving this open in case anyone else wants to chime in on this. http://gerrit.cloudera.org:8080/#/c/15018/9/src/kudu/common/table_util.cc@44 PS9, Line 44: must be other characters after the first one, as the first period is treated as > nit: not begin or end with a period ('.')? They can technically end with a period if it's not the only period in the string. I think manually renaming the tables after temporarily disabling Ranger integration should be enough for this. http://gerrit.cloudera.org:8080/#/c/15018/9/src/kudu/common/table_util.cc@106 PS9, Line 106: > If we think this is not possible, maybe remove it? Done http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util.cc File src/kudu/common/table_util.cc: http://gerrit.cloudera.org:8080/#/c/15018/10/src/kudu/common/table_util.cc@29 PS10, Line 29: DEFINE_string(ranger_default_database, "default", > Why does this need to be configurable? Under what circumstances would 'defa If a user has a default.foobar and also a foobar Ranger couldn't distinguish between them and privileges granted on db=default table=foobar would be applied to both tables. In this case users may want to distinguish by using another default database name they haven't used before. -- 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: 11 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Thu, 06 Feb 2020 19:26:24 +0000 Gerrit-HasComments: Yes
