You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/03/30 01:24:19 UTC
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Dan Burkert has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9862
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
KUDU-2191 (7/n): HmsCatalog
This commit adds some important higher-level HMS machinery in the
HmsCatalog class. This class is responsible for translating Kudu
catalog information to the Hive format, thread safety, retries, Hive HA,
and configuration flags for the Hive integration.
A follow-up commit will integrate the HmsCatalog into the
CatalogManager, at which point many parts of the HMS integration will
become functional. Until that point this is still non-functional code.
Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
---
M src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hms_catalog-test.cc
A src/kudu/hms/hms_catalog.cc
A src/kudu/hms/hms_catalog.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
8 files changed, 1,093 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/9862/1
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9862
to look at the new patch set (#2).
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
KUDU-2191 (7/n): HmsCatalog
This commit adds some important higher-level HMS machinery in the
HmsCatalog class. This class is responsible for translating Kudu
catalog information to the Hive format, thread safety, retries, Hive HA,
and configuration flags for the Hive integration.
A follow-up commit will integrate the HmsCatalog into the
CatalogManager, at which point many parts of the HMS integration will
become functional. Until that point this is still non-functional code.
Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
---
M src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hms_catalog-test.cc
A src/kudu/hms/hms_catalog.cc
A src/kudu/hms/hms_catalog.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
8 files changed, 1,090 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/9862/2
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9862
to look at the new patch set (#3).
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
KUDU-2191 (7/n): HmsCatalog
This commit adds some important higher-level HMS machinery in the
HmsCatalog class. This class is responsible for translating Kudu
catalog information to the Hive format, thread safety, retries, Hive HA,
and configuration flags for the Hive integration.
A follow-up commit will integrate the HmsCatalog into the
CatalogManager, at which point many parts of the HMS integration will
become functional. Until that point this is still non-functional code.
Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
---
M src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hms_catalog-test.cc
A src/kudu/hms/hms_catalog.cc
A src/kudu/hms/hms_catalog.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
8 files changed, 1,108 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/9862/3
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 2:
This was split out from https://gerrit.cloudera.org/c/8312/. It contains the HmsCatalog abstraction. The gflags and especially the handling of hive_metastore_uris has changed since the previous change list.
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 02:44:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/CMakeLists.txt@85
PS2, Line 85: ADD_KUDU_TEST(hms_client-test RUN_SERIAL true NUM_SHARDS 4)
> Nit: should precede hms_client-test alphabetically.
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc
File src/kudu/hms/hms_catalog-test.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@168
PS2, Line 168: ASSERT_OK(hms_->Stop());
: ASSERT_OK(hms_client_->Stop());
> Don't the destructors do this automatically?
If they are stopped in the destructors then any errors are only logged, and won't result in the test failing. I think it's a bit better to be explicit here.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@242
PS2, Line 242: };
: INSTANTIATE_TEST_CASE_P(HmsCatalogTests, HmsCatalogTestParameterized,
: ::testing::Values(false, true));
:
: // Test creating, altering, and dropping a table with the HMS Catalog.
: TEST_P(HmsCatalogTestParameterized, TestTableLifecycle) {
> Add 'const' and use kVariableName notation, since these are constants.
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@263
PS2, Line 263: b.AddColumn("new_column", DataType::INT32);
> And then verify that the table is gone?
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@272
PS2, Line 272: NO_FATAL
> Could ASSERT_OK here too.
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@280
PS2, Line 280: const string kTableId = "table-id";
> To make it clearer that AlterTable will create the entry, could you assert
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@287
PS2, Line 287: // entry is created with the new (valid) name.
> And verify that the tables are gone? Or is that a given?
It's a given, since the HMS could never contain such a table.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@297
PS2, Line 297: // Drop a table containing a Hive incompatible character, and ensure it doesn't fail.
: ASSERT_OK(hms_catalog_->DropTable(kTableId, "foo.☃"));
:
: // Drop a table without a database, and ensure it doesn't fail.
: ASSERT_OK(hms_catalog_->DropTable(kTableId, "no_database"));
: }
:
: // Checks that Kudu tables will not replace or modify existing HMS entries th
> const and kName.
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@358
PS2, Line 358: // This depends on the Kudu table not actually existing in the HMS catalog.
> Verify that the table is gone?
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@383
PS2, Line 383: a", kTable
> Nit: and ensure that the same operations succeed
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc@65
PS2, Line 65: TAG_FLAG(hive_metastore_sasl_enabled, experimental);
> Nit: if this applies to hive_metastore_sasl_enabled, please move it to just
Done
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc@70
PS2, Line 70: TAG_FLAG(hive_metastore_retry_count, advanced);
: TAG_FLAG(hive_metastore_retry_count, experimental);
> FWIW, I think experimental implies advanced so I don't think you need to ta
I'm somewhat in favor of keeping the tag here, since eventually we'll want to go through and remove 'experimental' from all of the flags here when we're ready to declare the HMS integration production ready, but we'll want to keep this flag tagged as advanced, since I don't expect anyone to change it. If you feel strongly I can remove it here and below, though.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc@74
PS2, Line 74: DEFINE_int32(hive_metastore_send_timeout, 60,
: "Configures the socket send timeout, in seconds, for Thrift "
: "connections to the Hive Metastore.");
: TAG_FLAG(hive_metastore_send_timeout, advanced);
: TAG_FLAG(hive_metastore_send_timeout, experimental);
: TAG_FLAG(hive_metastore_send_timeout, runtime);
:
: DEFINE_int32(hive_metastore_recv_timeout, 60,
: "Configures the socket receive timeout, in seconds, for Thrift "
: "connections to the Hive Metastore.");
: TAG_FLAG(hive_metastore_recv_timeout, advanced);
: TAG_FLAG(hive_metastore_recv_timeout, experimental);
: TAG_FLAG(hive_metastore_recv_timeout, runtime);
:
: DEFINE_int32(hive_metastore_conn_timeout, 60,
: "Configures the socket connect timeout, in seconds, for Thrift "
: "connections to the Hive Metastore.");
: TAG_FLAG(hive_metastore_conn_timeout, advanced);
: TAG_FLAG(hive_metastore_conn_timeout, experimental);
: TAG_FLAG(hive_metastore_conn_timeout, runtime);
> Since these are experimental and unlikely to be used, perhaps we can start
I think it's worth adding all three, since going back and adding them later on top of a single flag which controls all three will be messy. I expect that these will end up being useful as a workaround for slow HMS instances.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc@104
PS2, Line 104: tore_s
> Not sure I follow; if it were in the 'server' module, it would apply to tab
I'm referencing the 'server' module here because that's where --keytab-file is defined. It's 'weird' that we're doing validation on these two flags in the master module, since neither flag is defined there, however it's the LUB module that depends on both hms and server.
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 19:01:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
KUDU-2191 (7/n): HmsCatalog
This commit adds some important higher-level HMS machinery in the
HmsCatalog class. This class is responsible for translating Kudu
catalog information to the Hive format, thread safety, retries, Hive HA,
and configuration flags for the Hive integration.
A follow-up commit will integrate the HmsCatalog into the
CatalogManager, at which point many parts of the HMS integration will
become functional. Until that point this is still non-functional code.
Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Reviewed-on: http://gerrit.cloudera.org:8080/9862
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hms_catalog-test.cc
A src/kudu/hms/hms_catalog.cc
A src/kudu/hms/hms_catalog.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
8 files changed, 1,111 insertions(+), 2 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9862
to look at the new patch set (#5).
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
KUDU-2191 (7/n): HmsCatalog
This commit adds some important higher-level HMS machinery in the
HmsCatalog class. This class is responsible for translating Kudu
catalog information to the Hive format, thread safety, retries, Hive HA,
and configuration flags for the Hive integration.
A follow-up commit will integrate the HmsCatalog into the
CatalogManager, at which point many parts of the HMS integration will
become functional. Until that point this is still non-functional code.
Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
---
M src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hms_catalog-test.cc
A src/kudu/hms/hms_catalog.cc
A src/kudu/hms/hms_catalog.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
8 files changed, 1,111 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/9862/5
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc@104
PS2, Line 104: tore_s
> OK, maybe some more detail would help:
Sorry I forgot to mention that I did end up adding more color to the comment in R3. It doesn't look exactly like your text so let me know if I should wholesale copy it.
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 21:13:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 2:
And a bunch of tests were added to hms_catalog-test
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 02:45:03 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc@104
PS2, Line 104: tore_s
> Sorry I forgot to mention that I did end up adding more color to the commen
Nah your explanation is fine, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 22:42:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 2:
(14 comments)
The IWYU failures are weird; could you bug Alexey?
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/CMakeLists.txt@85
PS2, Line 85: ADD_KUDU_TEST(hms_catalog-test RUN_SERIAL true NUM_SHARDS 4)
Nit: should precede hms_client-test alphabetically.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc
File src/kudu/hms/hms_catalog-test.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@168
PS2, Line 168: ASSERT_OK(hms_->Stop());
: ASSERT_OK(hms_client_->Stop());
Don't the destructors do this automatically?
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@242
PS2, Line 242: string table_id = "table-id";
: string hms_database_name = "default";
: string hms_table_name = "table_name";
: string table_name = Substitute("$0.$1", hms_database_name, hms_table_name);
: string hms_altered_table_name = "altered_table_name";
: string altered_table_name = Substitute("$0.$1", hms_database_name, hms_altered_table_name);
Add 'const' and use kVariableName notation, since these are constants.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@263
PS2, Line 263: ASSERT_OK(hms_catalog_->DropTable(table_id, altered_table_name));
And then verify that the table is gone?
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@272
PS2, Line 272: CHECK_OK
Could ASSERT_OK here too.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@280
PS2, Line 280: ASSERT_OK(hms_client_->GetTable("default", "foo", &table));
To make it clearer that AlterTable will create the entry, could you assert that it doesn't exist before the alter?
Below as well.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@287
PS2, Line 287: // Drop a table containing a non Hive-compatible character, and ensure it doesn't fail.
And verify that the tables are gone? Or is that a given?
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@297
PS2, Line 297: string table_id = "abc123";
: string hms_database = "default";
:
: string hms_external_table = "external_table";
: string external_table_name = Substitute("$0.$1", hms_database, hms_external_table);
:
: string hms_kudu_table = "kudu_table";
: string kudu_table_name = Substitute("$0.$1", hms_database, hms_kudu_table);
const and kName.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@358
PS2, Line 358: // Drop a Kudu table with no corresponding HMS entry.
Verify that the table is gone?
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog-test.cc@383
PS2, Line 383: operations
Nit: and ensure that the same operations succeed
It's a bit pedantic, but it's important for these to be constant to prove that only one thing has changed: the status of the HMS.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc@65
PS2, Line 65: // Validated in master.cc.
Nit: if this applies to hive_metastore_sasl_enabled, please move it to just before the definition rather than just after.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc@70
PS2, Line 70: TAG_FLAG(hive_metastore_retry_count, advanced);
: TAG_FLAG(hive_metastore_retry_count, experimental);
FWIW, I think experimental implies advanced so I don't think you need to tag with both.
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/hms/hms_catalog.cc@74
PS2, Line 74: DEFINE_int32(hive_metastore_send_timeout, 60,
: "Configures the socket send timeout, in seconds, for Thrift "
: "connections to the Hive Metastore.");
: TAG_FLAG(hive_metastore_send_timeout, advanced);
: TAG_FLAG(hive_metastore_send_timeout, experimental);
: TAG_FLAG(hive_metastore_send_timeout, runtime);
:
: DEFINE_int32(hive_metastore_recv_timeout, 60,
: "Configures the socket receive timeout, in seconds, for Thrift "
: "connections to the Hive Metastore.");
: TAG_FLAG(hive_metastore_recv_timeout, advanced);
: TAG_FLAG(hive_metastore_recv_timeout, experimental);
: TAG_FLAG(hive_metastore_recv_timeout, runtime);
:
: DEFINE_int32(hive_metastore_conn_timeout, 60,
: "Configures the socket connect timeout, in seconds, for Thrift "
: "connections to the Hive Metastore.");
: TAG_FLAG(hive_metastore_conn_timeout, advanced);
: TAG_FLAG(hive_metastore_conn_timeout, experimental);
: TAG_FLAG(hive_metastore_conn_timeout, runtime);
Since these are experimental and unlikely to be used, perhaps we can start by providing just one flag that controls all three timeouts? Or do you think there's value in customizing each one separately from the get go?
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc@104
PS2, Line 104: server
Not sure I follow; if it were in the 'server' module, it would apply to tablet servers too, which isn't what we want.
Maybe you meant 'master' here? But then I don't understand the vice-versa comment, because doesn't the master module depend on kudu_hms?
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 17:31:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9862 )
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/9862/2/src/kudu/master/master.cc@104
PS2, Line 104: tore_s
> I'm referencing the 'server' module here because that's where --keytab-file
OK, maybe some more detail would help:
// Validates that if the HMS is configured with SASL enabled, the server has a
// keytab available.
//
// Note: neither of these flags are defined by the 'master' module, and yet this
// validation is performed here because this is the least upper bound between
// 'server' (where --keytab-file is defined) and 'kudu_hms' (where
// --hive_metastore_sasl_enabled) is defined.
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 19:33:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (7/n): HmsCatalog
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9862
to look at the new patch set (#6).
Change subject: KUDU-2191 (7/n): HmsCatalog
......................................................................
KUDU-2191 (7/n): HmsCatalog
This commit adds some important higher-level HMS machinery in the
HmsCatalog class. This class is responsible for translating Kudu
catalog information to the Hive format, thread safety, retries, Hive HA,
and configuration flags for the Hive integration.
A follow-up commit will integrate the HmsCatalog into the
CatalogManager, at which point many parts of the HMS integration will
become functional. Until that point this is still non-functional code.
Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
---
M src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hms_catalog-test.cc
A src/kudu/hms/hms_catalog.cc
A src/kudu/hms/hms_catalog.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
8 files changed, 1,111 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/9862/6
--
To view, visit http://gerrit.cloudera.org:8080/9862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6eb2a4c400f5aaee095e4e4ad572981565a1c040
Gerrit-Change-Number: 9862
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Todd Lipcon <to...@apache.org>