Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util-test.cc File src/kudu/util/table_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util-test.cc@54 PS1, Line 54: .no_table "no_table."? http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.h File src/kudu/util/table_util.h: PS1: nit: How would you feel about putting this in /common instead of /util? Looking over the rest of /util, it's mostly utility structs (eg Status, Once, Flags, etc); whereas /common seems to have more generic table/row-related utility functions. http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.h@19 PS1, Line 19: #include <string> Should be able to forward declare this http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.h@29 PS1, Line 29: Hive What's special about Hive database and Hive table names? Also, what is the expected Kudu table format? May be worth exposing this more transparently, e.g.: "Takes a table of the form "<database>.<table>" and returns Slices containing "<database>" and "<table>". Returns an error if the input table name is not correctly formatted." Ah, seems it's in the .cc. Maybe put it here so callers can just look at the header to figure out how this should be used. -- To view, visit http://gerrit.cloudera.org:8080/11658 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc Gerrit-Change-Number: 11658 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Fri, 12 Oct 2018 23:25:11 +0000 Gerrit-HasComments: Yes
