You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2019/05/15 22:19:06 UTC
[kudu-CR] POC hms: allow and ignore all events from non-managed tables
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13347
Change subject: POC hms: allow and ignore all events from non-managed tables
......................................................................
POC hms: allow and ignore all events from non-managed tables
This patch explores the work needed to ignore
non-managed Kudu tables in the KuduMetastorePlugin
and hms_notification_log_listener. The primary goal
is to ignore tables with the type EXTERNAL_TABLE.
But other unexpected and unhandled table types such as VIRTUAL_VIEW, INDEX_TABLE, and MATERIALIZED_VIEW are also ignored.
There are TODOs around open questions for policies we
may want to enforce and testing required.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
4 files changed, 51 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/1
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 4:
(4 comments)
> Patch Set 4:
>
> It looks like I need to make some tooling changes to fix this yet.
Yeah, the change in PopulateTables affects the tooling behavior :/
http://gerrit.cloudera.org:8080/#/c/13347/2//COMMIT_MSG
Commit Message:
PS2:
> I thought about breaking this apart, but both pieces can't function without
Yeah, SGTM.
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
File java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java:
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@149
PS4, Line 149: isKuduTable(oldTable)
This is allowing for table type alterations on legacy metadata, and allowing upgrades from legacy external to new-world managed. I think that's intentional, maybe mention that these cases are explicitly left available?
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/hms/hms_catalog.cc@a347
PS4, Line 347:
:
Should we be explicit and set this to "FALSE"? Or explicitly erase the property if it exists?
That may have implications on the tooling side.
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/kudu-tool-test.cc@a3266
PS4, Line 3266:
Now, we expect this to always exist, right (instead of never)?
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 20 May 2019 21:21:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] POC hms: ignore external tables and handle new StorageHandler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#2).
Change subject: POC hms: ignore external tables and handle new StorageHandler
......................................................................
POC hms: ignore external tables and handle new StorageHandler
This enables the HMS integration to ignore
non-managed Kudu tables in the KuduMetastorePlugin
and hms_notification_log_listener.
The primary goal is to ignore tables with the type
EXTERNAL_TABLE. But other unexpected and unhandled
table types such as VIRTUAL_VIEW, INDEX_TABLE, and
MATERIALIZED_VIEW are also ignored.
Additionally it updates the server code to support
using the new StorageHandler for all new tables.
TODO: Tooling changes
TODO: Patch breakout
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
13 files changed, 205 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/2
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#6).
Change subject: hms: Adjust external table handling
......................................................................
hms: Adjust external table handling
This patch changes the handling of external HMS tables.
Currently we synchronize external HMS entries and
create all synchronized HMS entries as external.
With this patch, we only synchronize managed HMS
entries and create all entries as managed by default.
This allows external tables to work the same regardless
of HMS synchronization being enabled.
This patch touches many locations in the code because
the use and validation of the new behavior is tightly
coupled.
The changes include:
- Prevent altering table type via the KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
hms_notification_log_listener
- Use managed tables by default
- Always use the new storage format for new entries
- Add the table_name property to the new storage format
- Adjust upgrade and downgrade logic for external tables
- Adjust HmsCatalog::PopulateTable logic for external
tables
- Adjust the tooling logic to correctly handle external
tables.
Note: The updates to tooling logic and testing was
minimal and will be improved in follow on patches.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
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/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
14 files changed, 386 insertions(+), 205 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/6
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#10).
Change subject: hms: Adjust external table handling
......................................................................
hms: Adjust external table handling
This patch changes the handling of external HMS tables.
Currently we synchronize external HMS entries and
create all synchronized HMS entries as external.
With this patch, we only synchronize managed HMS
entries and create all entries as managed by default.
This allows external tables to work the same regardless
of HMS synchronization being enabled.
This patch touches many locations in the code because
the use and validation of the new behavior is tightly
coupled.
The changes include:
- Prevent altering table type via the KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
hms_notification_log_listener
- Use managed tables by default
- Always use the new storage format for new entries
- Add the table_name property to the new storage format
- Adjust upgrade and downgrade logic for external tables
- Adjust HmsCatalog::PopulateTable logic for external
tables
- Adjust the tooling logic to correctly handle external
tables.
Note: The updates to tooling logic and testing was
minimal and will be improved in follow on patches.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
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/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
14 files changed, 381 insertions(+), 190 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/10
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: hms: Adjust external table handling
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
hms: Adjust external table handling
This patch changes the handling of external HMS tables.
Currently we synchronize external HMS entries and
create all synchronized HMS entries as external.
With this patch, we only synchronize managed HMS
entries and create all entries as managed by default.
This allows external tables to work the same regardless
of HMS synchronization being enabled.
This patch touches many locations in the code because
the use and validation of the new behavior is tightly
coupled.
The changes include:
- Prevent altering table type via the KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
hms_notification_log_listener
- Use managed tables by default
- Always use the new storage format for new entries
- Add the table_name property to the new storage format
- Adjust upgrade and downgrade logic for external tables
- Adjust HmsCatalog::PopulateTable logic for external
tables
- Adjust the tooling logic to correctly handle external
tables.
Note: The updates to tooling logic and testing was
minimal and will be improved in follow on patches.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Reviewed-on: http://gerrit.cloudera.org:8080/13347
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
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/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
14 files changed, 381 insertions(+), 190 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved; Verified
Mike Percy: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 9:
(5 comments)
Overall looks pretty good to me, although this is the first time I have looked at the code being altered here. I just have some minor feedback and questions.
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java:
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@354
PS9, Line 354: client.alter_table(table.getDbName(), table.getTableName(), table);
Is anything actually being altered here? Doesn't seem to be testing much. Maybe we should alter the HMS table name, kudu.table_name property, etc
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.h@68
PS9, Line 68: const std::string& table_type = hms::HmsClient::kManagedTable)
Just wondering: when would Kudu create anything other than a managed HMS table? Or is this just here to maintain expressiveness in the API?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.h@146
PS9, Line 146: table_type
Just wondering, do we ever want to use PopulateTable() to populate a hive::Table object for an external HMS table?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.cc@360
PS9, Line 360: if
nit: } else if, or ideally switch
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@347
PS9, Line 347: vector<pair<shared_ptr<KuduTable>, hive::Table>> duplicate_tables;
Seems like we should have a TODO or a Jira to update the duplicate handling since duplicates / aliases will still be allowed going forward.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 20:45:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 9:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java:
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@250
PS9, Line 250: // Try to alter the Kudu table to a different type.
: try {
: Table alteredTable = table.deepCopy();
: alteredTable.setTableType(TableType.EXTERNAL_TABLE.toString());
: client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
: fail();
: } catch (TException e) {
: assertTrue(e.getMessage().contains(
: "Kudu table type may not be altered"));
: }
> Could you add the opposite test, going from EXTERNAL --> MANAGED?
It takes a bit more setup and feels a bit overkill given the code blocks all type changes (including VIRTUAL_VIEW, INDEX_TABLE, MATERIALIZED_VIEW).
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@354
PS9, Line 354: client.alter_table(table.getDbName(), table.getTableName(), table);
> Is anything actually being altered here? Doesn't seem to be testing much. M
It doesn't need to test the contents of an alter, just that the plugin accepts it. This plugin is only meant for validation and acceptance of alter events to the HMS.
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@420
PS9, Line 420: // Test dropping a non-managed table is accepted.
: {
: table.getParameters().clear();
: table.setTableType(TableType.EXTERNAL_TABLE.name());
: client.createTable(table);
: client.dropTable(table.getDbName(), table.getTableName());
: }
> I get why this is here, because of how these tests are split out into indiv
True, 213 exercises this due to the created table needing to be dropped. I would prefer to handle more wholistic testing improvements into a follow on patch.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.h@68
PS9, Line 68: const std::string& table_type = hms::HmsClient::kManagedTable)
> Just wondering: when would Kudu create anything other than a managed HMS ta
This is primarily used in testing to create external HMS entries.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.h@146
PS9, Line 146: table_type
> Just wondering, do we ever want to use PopulateTable() to populate a hive::
We use this a bit in the tooling. One example is upgrading external Kudu tables. Populate table is used to build all HMS table representations Kudu side.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/hms/hms_catalog.cc@360
PS9, Line 360: if
> nit: } else if, or ideally switch
changed to an else if, will clean up some stylistic things like this in a follow up.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/hms_itest-base.cc
File src/kudu/integration-tests/hms_itest-base.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/hms_itest-base.cc@168
PS9, Line 168: hms_table.tableType, hms::HmsClient::kManagedTable);
:
> nit: this should be reversed so the expected value is first
Done
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/master_hms-itest.cc@216
PS9, Line 216: // Rename the external table through the HMS and ensure Kudu allows it.
: ASSERT_OK(RenameHmsTable("db", "b", "e"));
> Should we test that the underlying Kudu table is unchanged?
There is no underlying Kudu table, db.b is an external non-Kudu table in this case.
We can add more test cases in follow up patches.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3275
PS9, Line 3275: ASSERT_EQ(hms_table.parameters[HmsClient::kKuduTableIdKey], kudu_table->id());
> nit: reversed order here too
Done
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3280
PS9, Line 3280: ASSERT_TRUE
> nit: could use ASSERT_FALSE() here
Done
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3415
PS9, Line 3415:
: // Test case: legacy external table (pointed at the legacy managed table).
: ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_external",
: "impala::default.legacy_managed", master_addr, HmsClient::kExternalTable, kUsername));
> I think I'm missing something. Why is this considered an inconsistent table
Because it is using the legacy storage handler. See below where it's made consistent via "Upgrade legacy HMS tables."
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@306
PS9, Line 306: for (hive::Table& hms_table : hms_tables) {
: const string &storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
: // If this is a legacy table or external table, lookup the Kudu table by name.
: if (storage_handler == HmsClient::kLegacyKuduStorageHandler ||
: hms_table.tableType == HmsClient::kExternalTable) {
> nit: May be simpler as:
I agree this logic can be simplified and cleaned up. Based on my TODO's below it will change. Mind if I do this cleanup in that patch?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@312
PS9, Line 312: *
> nit: reversed spacing, here and L311 and L331 and L332
Done
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@347
PS9, Line 347: vector<pair<shared_ptr<KuduTable>, hive::Table>> duplicate_tables;
> Seems like we should have a TODO or a Jira to update the duplicate handling
This handling is correct. We break out managed_hms_tables_by_id and external_hms_tables_by_id. These duplicates are only for managed tables.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@440
PS9, Line 440: cout << "Found Kudu table(s) with multiple corresponding Hive Metastore tables:" << endl;
> The plugin disallows that transition now, doesn't it?
This can still happen before the HMS integration is enabled or if it is ever disabled.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 21:41:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#8).
Change subject: hms: Adjust external table handling
......................................................................
hms: Adjust external table handling
This patch changes the handling of external HMS tables.
Currently we synchronize external HMS entries and
create all synchronized HMS entries as external.
With this patch, we only synchronize managed HMS
entries and create all entries as managed by default.
This allows external tables to work the same regardless
of HMS synchronization being enabled.
This patch touches many locations in the code because
the use and validation of the new behavior is tightly
coupled.
The changes include:
- Prevent altering table type via the KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
hms_notification_log_listener
- Use managed tables by default
- Always use the new storage format for new entries
- Add the table_name property to the new storage format
- Adjust upgrade and downgrade logic for external tables
- Adjust HmsCatalog::PopulateTable logic for external
tables
- Adjust the tooling logic to correctly handle external
tables.
Note: The updates to tooling logic and testing was
minimal and will be improved in follow on patches.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
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/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
14 files changed, 388 insertions(+), 205 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/8
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#4).
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
hms: ignore external tables and handle new StorageHandler
This enables the HMS integration to ignore
non-managed Kudu tables in the KuduMetastorePlugin
and hms_notification_log_listener.
The primary goal is to ignore tables with the type
EXTERNAL_TABLE. But other unexpected and unhandled
table types such as VIRTUAL_VIEW, INDEX_TABLE, and
MATERIALIZED_VIEW are also ignored.
Additionally it updates the server code to support
using the new StorageHandler for all new tables.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
13 files changed, 227 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/4
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
File java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java:
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@149
PS4, Line 149: isKuduTable(oldTable)
> This is allowing for table type alterations on legacy metadata, and allowin
Yep, I'm also curious what if they try to turn non-Kudu table into a managed Kudu table? Will it work as expected or it's not allowed due to some other restrictions?
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@168
PS4, Line 168: !
Not a part of this changelist, but I'm curious whether this logic matches the comment for this if() clause.
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/master/hms_notification_log_listener.cc@337
PS4, Line 337: // Not a managed table; skip it.
: return Status::OK();
Do you think it's worth adding VLOG(2) or something alike for troubleshooting purposes?
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/tool_action_hms.cc@321
PS4, Line 321: }
Not a part of this patch, but what if the handler is neither of LegacyStorageHandler nor StorageHandler? Maybe, at least add LOG(WARNING) about that condition? Imagine this old tool is run against some new Kudu where HMS tables have richer set of possible types?
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 20 May 2019 23:30:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13347/2//COMMIT_MSG
Commit Message:
PS2:
> Actually we wouldn't be able to merge the plugin changes without the others
I thought about breaking this apart, but both pieces can't function without the other. Given the patch is relatively small, I think we should keep it together and review as is. The tooling work can be done in another patch.
This patch supersedes https://gerrit.cloudera.org/c/13315 and has different functionality because of recent design decisions.
I think https://gerrit.cloudera.org/#/c/13314/ can be rebased on this patch. I expect the tooling work to need a bunch of changes based on the new design changes.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 20 May 2019 00:23:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 14:11:12 +0000
Gerrit-HasComments: No
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#9).
Change subject: hms: Adjust external table handling
......................................................................
hms: Adjust external table handling
This patch changes the handling of external HMS tables.
Currently we synchronize external HMS entries and
create all synchronized HMS entries as external.
With this patch, we only synchronize managed HMS
entries and create all entries as managed by default.
This allows external tables to work the same regardless
of HMS synchronization being enabled.
This patch touches many locations in the code because
the use and validation of the new behavior is tightly
coupled.
The changes include:
- Prevent altering table type via the KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
hms_notification_log_listener
- Use managed tables by default
- Always use the new storage format for new entries
- Add the table_name property to the new storage format
- Adjust upgrade and downgrade logic for external tables
- Adjust HmsCatalog::PopulateTable logic for external
tables
- Adjust the tooling logic to correctly handle external
tables.
Note: The updates to tooling logic and testing was
minimal and will be improved in follow on patches.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
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/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
14 files changed, 386 insertions(+), 204 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/9
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#7).
Change subject: hms: Adjust external table handling
......................................................................
hms: Adjust external table handling
This patch changes the handling of external HMS tables.
Currently we synchronize external HMS entries and
create all synchronized HMS entries as external.
With this patch, we only synchronize managed HMS
entries and create all entries as managed by default.
This allows external tables to work the same regardless
of HMS synchronization being enabled.
This patch touches many locations in the code because
the use and validation of the new behavior is tightly
coupled.
The changes include:
- Prevent altering table type via the KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
KuduMetastorePlugin
- Ignore non-managed Kudu tables in the
hms_notification_log_listener
- Use managed tables by default
- Always use the new storage format for new entries
- Add the table_name property to the new storage format
- Adjust upgrade and downgrade logic for external tables
- Adjust HmsCatalog::PopulateTable logic for external
tables
- Adjust the tooling logic to correctly handle external
tables.
Note: The updates to tooling logic and testing was
minimal and will be improved in follow on patches.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
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/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
14 files changed, 386 insertions(+), 205 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/7
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: hms: Adjust external table handling
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13347
to look at the new patch set (#3).
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
hms: ignore external tables and handle new StorageHandler
This enables the HMS integration to ignore
non-managed Kudu tables in the KuduMetastorePlugin
and hms_notification_log_listener.
The primary goal is to ignore tables with the type
EXTERNAL_TABLE. But other unexpected and unhandled
table types such as VIRTUAL_VIEW, INDEX_TABLE, and
MATERIALIZED_VIEW are also ignored.
Additionally it updates the server code to support
using the new StorageHandler for all new tables.
Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
---
M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_hms.cc
13 files changed, 226 insertions(+), 116 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13347/3
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] POC hms: ignore external tables and handle new StorageHandler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: POC hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13347/2//COMMIT_MSG
Commit Message:
PS2:
> This is great to get a full picture of the necessary changes to do this, bu
Actually we wouldn't be able to merge the plugin changes without the others.. Still I agree with your TODO to break this apart.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 18 May 2019 22:27:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 22:23:03 +0000
Gerrit-HasComments: No
[kudu-CR] hms: Adjust external table handling
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 10: Verified+1 Code-Review+2
(3 comments)
3 unrelated flaky java tests.
LGTM, given we're trying to unblock other work, and there are follow-up tests slated.
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3415
PS9, Line 3415: SchemaBuilder().Build()));
:
: // Test case: inconsistent name.
: shared_ptr<KuduTable> inconsistent_name;
> Because it is using the legacy storage handler. See below where it's made c
Makes sense. Orthogonal but related: was the change from "default.legacy_external" to "legacy_external" required?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@306
PS9, Line 306: for (hive::Table& hms_table : hms_tables) {
: const string &storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
: // If this is a legacy table or external table, lookup the Kudu table by name.
: if (storage_handler == HmsClient::kLegacyKuduStorageHandler ||
: hms_table.tableType == HmsClient::kExternalTable) {
> I agree this logic can be simplified and cleaned up. Based on my TODO's bel
SGTM
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@440
PS9, Line 440: cout << "Found Kudu table(s) with multiple corresponding Hive Metastore tables:" << endl;
> This can still happen before the HMS integration is enabled or if it is eve
Sure, just making sure I understand the procedure. In this context, because it's a function of the plugin, that means removing the plugin from the HMS, rather than just disabling via gflags.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 22:08:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 9:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
File java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java:
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@149
PS4, Line 149: ote: This doesn't prev
> Yep, I'm also curious what if they try to turn non-Kudu table into a manage
yeah, this isn't meant to impact the behavior of legacy tables at all. I will make that explicit in a comment.
http://gerrit.cloudera.org:8080/#/c/13347/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@168
PS4, Line 168:
> Not a part of this changelist, but I'm curious whether this logic matches t
This is addressed in Hao's patches.
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/hms/hms_catalog.cc@a347
PS4, Line 347:
:
> Should we be explicit and set this to "FALSE"? Or explicitly erase the prop
I don't think we should explicitly set it. We can fallback on any default HMS handling. I will make sure it's erased if this is a managed table.
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/master/hms_notification_log_listener.cc@337
PS4, Line 337: // Not a managed table; skip it.
: VLOG(2) << Substitut
> Do you think it's worth adding VLOG(2) or something alike for troubleshooti
Done
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/kudu-tool-test.cc@a3266
PS4, Line 3266:
> Now, we expect this to always exist, right (instead of never)?
Right I need to make a bunch of tooling changes.
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/tool_action_hms.cc@321
PS4, Line 321: }
> Not a part of this patch, but what if the handler is neither of LegacyStorageHandler nor StorageHandler? Maybe, at least add LOG(WARNING) about that condition?
In that case those tables are considered to not be Kudu tables and they are silently ignored. there are lots of StorageHandlers in the HMS, logging here would result in logging all non-Kudu tables. See `hms_catalog->GetKuduTables`.
> Imagine this old tool is run against some new Kudu where HMS tables have richer set of possible types?
We always expect the Kudu tool version to be >= the Kudu version.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 14:10:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 10: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/4/src/kudu/tools/tool_action_hms.cc@321
PS4, Line 321: }
> > Not a part of this patch, but what if the handler is neither of LegacySto
SGTM. Indeed, that sounds like a hypothetical scenario, actually. Keeping versions in sync will surely help in avoiding extra-cruft elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 22:30:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 9:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java:
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@250
PS9, Line 250: // Try to alter the Kudu table to a different type.
: try {
: Table alteredTable = table.deepCopy();
: alteredTable.setTableType(TableType.EXTERNAL_TABLE.toString());
: client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
: fail();
: } catch (TException e) {
: assertTrue(e.getMessage().contains(
: "Kudu table type may not be altered"));
: }
Could you add the opposite test, going from EXTERNAL --> MANAGED?
http://gerrit.cloudera.org:8080/#/c/13347/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@420
PS9, Line 420: // Test dropping a non-managed table is accepted.
: {
: table.getParameters().clear();
: table.setTableType(TableType.EXTERNAL_TABLE.name());
: client.createTable(table);
: client.dropTable(table.getDbName(), table.getTableName());
: }
I get why this is here, because of how these tests are split out into individual DDL, but it's also at L213, so this can be removed?
Alternatively, break out a test case to test external tables explicitly and put _all_ external table-related testing in that one case? That way it's clear where to look for externality testing (rather than having it be smeared across this test).
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/hms_itest-base.cc
File src/kudu/integration-tests/hms_itest-base.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/hms_itest-base.cc@168
PS9, Line 168: hms_table.tableType, hms::HmsClient::kManagedTable);
:
nit: this should be reversed so the expected value is first
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/master_hms-itest.cc@216
PS9, Line 216: // Rename the external table through the HMS and ensure Kudu allows it.
: ASSERT_OK(RenameHmsTable("db", "b", "e"));
Should we test that the underlying Kudu table is unchanged?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/integration-tests/master_hms-itest.cc@327
PS9, Line 327: // Create and drop a non-Kudu ('external') HMS table entry and ensure Kudu allows it.
: ASSERT_OK(CreateHmsTable("default", "externalTable", HmsClient::kExternalTable));
: ASSERT_OK(hms_client_->DropTable("default", "externalTable"));
Should we test pointing this at a real table and verifying that the underlying table doesn't get dropped?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3275
PS9, Line 3275: ASSERT_EQ(hms_table.parameters[HmsClient::kKuduTableIdKey], kudu_table->id());
nit: reversed order here too
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3280
PS9, Line 3280: ASSERT_TRUE
nit: could use ASSERT_FALSE() here
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3415
PS9, Line 3415:
: // Test case: legacy external table (pointed at the legacy managed table).
: ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_external",
: "impala::default.legacy_managed", master_addr, HmsClient::kExternalTable, kUsername));
I think I'm missing something. Why is this considered an inconsistent table?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@306
PS9, Line 306: for (hive::Table& hms_table : hms_tables) {
: const string &storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
: // If this is a legacy table or external table, lookup the Kudu table by name.
: if (storage_handler == HmsClient::kLegacyKuduStorageHandler ||
: hms_table.tableType == HmsClient::kExternalTable) {
nit: May be simpler as:
shared_ptr<KuduTable> kudu_table;
if (storage_handler == HmsClient::kKuduStorageHandler && hms_table.tableType == HmsClient::kManagedTable) {
// lookup by ID
} else {
// lookup by name
// this could also _not_ be an "else" and could be adjusted to handle the TODOs
}
if (kudu_table) {
if (hms_table.tableType == HmsClient::ExternalTable) {
// add to external_hms_tables_by_id
} else {
// add to managed_hms_tables_by_id
}
} else {
// if managed, add to orphan_hms_tables
}
WDYT?
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@312
PS9, Line 312: *
nit: reversed spacing, here and L311 and L331 and L332
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@440
PS9, Line 440: cout << "Found Kudu table(s) with multiple corresponding Hive Metastore tables:" << endl;
The plugin disallows that transition now, doesn't it?
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 20:55:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 5:
Rebased this to pile some flaky test handling onto it.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 21 May 2019 00:31:41 +0000
Gerrit-HasComments: No
[kudu-CR] POC hms: ignore external tables and handle new StorageHandler
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: POC hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13347/2//COMMIT_MSG
Commit Message:
PS2:
This is great to get a full picture of the necessary changes to do this, but this should probably be broken up into its logical pieces so we make sure each piece gets ample test coverage and attention.
Also mentioned on Slack that this overlaps directly with two such existing patches that are being reviewed:
https://gerrit.cloudera.org/c/13315
https://gerrit.cloudera.org/c/13314
I noted in those that changing the default table type to managed _will_ make tests flaky, which is yet another reason to break this up a bit (e.g. could merge the plugin changes in isolation without having to fix the flaky tests).
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 18 May 2019 07:27:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: Adjust external table handling
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: Adjust external table handling
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/kudu-tool-test.cc@3415
PS9, Line 3415:
: // Test case: legacy external table (pointed at the legacy managed table).
: ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_external",
: "impala::default.legacy_managed", master_addr, HmsClient::kExternalTable, kUsername));
> Makes sense. Orthogonal but related: was the change from "default.legacy_ex
Yeah, "deafult.legacy_external" was the Kudu table name. But because this is external, and the new external behavior, there isn't a Kudu table anymore. The HMS table name is just "legacy_external".
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/13347/9/src/kudu/tools/tool_action_hms.cc@440
PS9, Line 440: cout << "Found Kudu table(s) with multiple corresponding Hive Metastore tables:" << endl;
> Sure, just making sure I understand the procedure. In this context, because
Either I suppose.
Duplicate managed tables can happen when a user alters a table to be external, points it to another managed kudu table, and alters it back to being managed. This isn't allowed when the plugin is being enforced. But this tool is for fixing scenarios like this.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 22 May 2019 22:13:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms: ignore external tables and handle new StorageHandler
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13347 )
Change subject: hms: ignore external tables and handle new StorageHandler
......................................................................
Patch Set 4:
It looks like I need to make some tooling changes to fix this yet.
--
To view, visit http://gerrit.cloudera.org:8080/13347
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
Gerrit-Change-Number: 13347
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 20 May 2019 18:30:04 +0000
Gerrit-HasComments: No