You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2018/10/11 07:18:24 UTC
[kudu-CR] [sentry] move ParseTableName to table util
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11658
Change subject: [sentry] move ParseTableName to table_util
......................................................................
[sentry] move ParseTableName to table_util
This commit moves HmsCatalog::ParseTableName to table_util. So that
other modules that need to parse a Kudu table name into a database and
table name can use it without including hms module, e.g sentry.
Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
---
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/table_util-test.cc
A src/kudu/util/table_util.cc
A src/kudu/util/table_util.h
8 files changed, 169 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11658/1
--
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: newchange
Gerrit-Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
Gerrit-Change-Number: 11658
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 12 Oct 2018 23:25:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h@34
PS3, Line 34: ParseHiveTableIdent
> Maybe just name it ParseHiveTableIdentifier?
SGTM
--
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: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:53:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 4: Code-Review+2
--
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: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:52:52 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 1:
(2 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@50
PS1, Line 50: EXPECT_TRUE(ParseTableName("no-table", &db, &tbl).IsInvalidArgument());
not your fault, but looks like these two test cases got duplicated between lines 50-53
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.cc
File src/kudu/util/table_util.cc:
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.cc@36
PS1, Line 36: Status ParseTableName(const string& table_name,
Given this is no longer 'scoped' under the hms module, I think it makes sense to change the name to 'ParseHiveTableIdent' or similar.
--
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 <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 12 Oct 2018 17:06:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 2:
(6 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@50
PS1, Line 50:
> not your fault, but looks like these two test cases got duplicated between
Done
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util-test.cc@54
PS1, Line 54:
> "no_table."?
Done
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? Loo
Done
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.h@19
PS1, Line 19:
> Should be able to forward declare this
If I remove it IWYU will throw an error.
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.h@29
PS1, Line 29:
> What's special about Hive database and Hive table names? Also, what is the
Done
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.cc
File src/kudu/util/table_util.cc:
http://gerrit.cloudera.org:8080/#/c/11658/1/src/kudu/util/table_util.cc@36
PS1, Line 36:
> Given this is no longer 'scoped' under the hms module, I think it makes sen
Done
--
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: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 15 Oct 2018 04:41:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 5:
(2 comments)
> Hmm, wish I got here earlier. I don't love the use of common for
> this; common is Kudu-specific common stuff (wire protocol, schema,
> row/column block layouts, etc.) and this is squarely a Hive thing.
>
> What do you guys think about moving it back to the hms module but
> making it a header-only inlined function so that consumers need not
> link against (and thus depend on) the hms module?
I am good to move it back to the hms module, just wondering whether this function is too large to be inlined?
http://gerrit.cloudera.org:8080/#/c/11658/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11658/3//COMMIT_MSG@9
PS3, Line 9: , so
> nit: use a comma instead
Done
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/integration-tests/master_hms-itest.cc@45
PS3, Line 45: #include "kudu/util/status.h"
> error: 'kudu/util/table_util.h' file not found [clang-diagnostic-error]
Done
--
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: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 18 Oct 2018 05:47:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11658/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11658/3//COMMIT_MSG@9
PS3, Line 9: . So
nit: use a comma instead
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h@34
PS3, Line 34: ParseHiveTableIdent
What does "Ident" mean in this context?
--
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: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 18:30:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h@34
PS3, Line 34: ParseHiveTableIdent
> Ah, gotcha. I thought it was "Identity" or something, which seemed kind of
Maybe just name it ParseHiveTableIdentifier?
--
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: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:32:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h@34
PS3, Line 34: ParseHiveTableIdent
> Maybe just name it ParseHiveTableIdentifier?
All of the suggestions work for me.
--
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: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:43:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11658
to look at the new patch set (#4).
Change subject: [sentry] move ParseTableName to table_util
......................................................................
[sentry] move ParseTableName to table_util
This commit moves HmsCatalog::ParseTableName to table_util, so that
other modules that need to parse a Kudu table name into a database and
table name can use it without including hms module, e.g sentry.
Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
---
M src/kudu/common/CMakeLists.txt
A src/kudu/common/table_util-test.cc
A src/kudu/common/table_util.cc
A src/kudu/common/table_util.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master_hms-itest.cc
8 files changed, 179 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11658/4
--
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: newpatchset
Gerrit-Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
Gerrit-Change-Number: 11658
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11658
to look at the new patch set (#2).
Change subject: [sentry] move ParseTableName to table_util
......................................................................
[sentry] move ParseTableName to table_util
This commit moves HmsCatalog::ParseTableName to table_util. So that
other modules that need to parse a Kudu table name into a database and
table name can use it without including hms module, e.g sentry.
Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
---
M src/kudu/common/CMakeLists.txt
A src/kudu/common/table_util-test.cc
A src/kudu/common/table_util.cc
A src/kudu/common/table_util.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master_hms-itest.cc
8 files changed, 182 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11658/2
--
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: newpatchset
Gerrit-Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
Gerrit-Change-Number: 11658
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
[sentry] move ParseTableName to table_util
This commit moves HmsCatalog::ParseTableName to table_util, so that
other modules that need to parse a Kudu table name into a database and
table name can use it without including hms module, e.g sentry.
Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
Reviewed-on: http://gerrit.cloudera.org:8080/11658
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/common/CMakeLists.txt
A src/kudu/common/table_util-test.cc
A src/kudu/common/table_util.cc
A src/kudu/common/table_util.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master_hms-itest.cc
8 files changed, 179 insertions(+), 86 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Andrew Wong: Looks good to me, approved
Kudu Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
Gerrit-Change-Number: 11658
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h@34
PS3, Line 34: ParseHiveTableIdent
> Short for 'identifier'. I suggested this name because it's parsing both th
Ah, gotcha. I thought it was "Identity" or something, which seemed kind of odd, but "Identifier" makes sense. Just adding the word "identifier" somewhere in the comment would help. Or maybe ParseHiveDbAndTable()? GetHiveDbAndTable()?
--
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: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:29:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/11658/3/src/kudu/common/table_util.h@34
PS3, Line 34: ParseHiveTableIdent
> What does "Ident" mean in this context?
Short for 'identifier'. I suggested this name because it's parsing both the table _and_ database name, and it was my best idea for how to relate that. Naming is hard, any suggestions?
--
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: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:23:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11658
to look at the new patch set (#3).
Change subject: [sentry] move ParseTableName to table_util
......................................................................
[sentry] move ParseTableName to table_util
This commit moves HmsCatalog::ParseTableName to table_util. So that
other modules that need to parse a Kudu table name into a database and
table name can use it without including hms module, e.g sentry.
Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
---
M src/kudu/common/CMakeLists.txt
A src/kudu/common/table_util-test.cc
A src/kudu/common/table_util.cc
A src/kudu/common/table_util.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master_hms-itest.cc
8 files changed, 179 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11658/3
--
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: newpatchset
Gerrit-Change-Id: I7fff5aaff623ac65a949f6cabebd9eca997f5cdc
Gerrit-Change-Number: 11658
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [sentry] move ParseTableName to table util
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11658 )
Change subject: [sentry] move ParseTableName to table_util
......................................................................
Patch Set 5:
Hmm, wish I got here earlier. I don't love the use of common for this; common is Kudu-specific common stuff (wire protocol, schema, row/column block layouts, etc.) and this is squarely a Hive thing.
What do you guys think about moving it back to the hms module but making it a header-only inlined function so that consumers need not link against (and thus depend on) the hms module?
--
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: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 17 Oct 2018 03:46:09 +0000
Gerrit-HasComments: No