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/09/05 17:42:18 UTC
[kudu-CR] [hms] Sync external HMS tables with purge property
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14179
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as syncronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type.
Last, the HMS tooling was updated to differentiate between syncronized
and unsyncronized tables instead of explicitly manged and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 226 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/1
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Greg Solovyev, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14179
to look at the new patch set (#6).
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type. The HMS plugin
tests were also adjusted to use external purged tables by default to avoid
testing issues one the auto-migration code is enabled in Hive.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 264 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/6
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14179
to look at the new patch set (#2).
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 230 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/2
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
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] [hms] Sync external HMS tables with purge property
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14179/2/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/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@265
PS2, Line 265:
nit: why 3 lines of space here?
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@319
PS2, Line 319: {
what's the point of {} here and above where there is no try-catch?
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 05 Sep 2019 20:59:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14179/4/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/14179/4/src/kudu/hms/hms_catalog.cc@189
PS4, Line 189: // If this is an external table, only upgrade the storage handler.
: if (table.tableType == HmsClient::kExternalTable) {
: table.parameters[HmsClient::kStorageHandlerKey] = HmsClient::kKuduStorageHandler;
: }
> Sorry missed this in last review, do we need to check for external.table.pu
Done
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 Sep 2019 13:43:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 2:
(27 comments)
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG@12
PS1, Line 12: Further, in
: Hive 4, non-transactional Hive tables will get auto-migrated
: to external tables with `external.table.purge=true`.
> So with this patch, HMS sync is compliant with Hive 3 but not with Hive 4?
With this patch we should be compliant with Hive 2, 3, and 4. Because HMS sync will now synchronize the managed table type and external table type with the property `external.table.purge=true`.
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG@17
PS1, Line 17: synchronize
> synchronized
Done
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG@26
PS1, Line 26: manag
> managed
Done
http://gerrit.cloudera.org:8080/#/c/14179/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14179/2//COMMIT_MSG@13
PS2, Line 13: Hive tables
> Does it matter if they are managed or external originally?
external tables don't need to be migrated. The migration here is from managed to external tables.
http://gerrit.cloudera.org:8080/#/c/14179/1/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/14179/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@187
PS1, Line 187: oldTableType, newTable.
> Should use oldTableType here.
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/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/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@183
PS2, Line 183: // The Kudu master is allowed to make these changes if necessary as it is a trusted user.
> Is this in preparation for introducing tooling to migrate managed-->externa
Yeah, there are a few reasons for this. The first was to support testing. In versions where the auto-migration code in Hive doesn't exist, pre-hive4, I need to be able to create and alter external purge tables to test the handling.
In the future we may want to migrate managed tables to external purge tables using the tools and this will be useful for that too.
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@194
PS2, Line 194: !TableType.MANAGED_TABLE.name().equals(oldTableType) &&
: !(isExternalTable(oldTable) &&
> Isn't some or all of this taken care of by isPurgeTable?
replaced with `isSynchronizedTable`
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@279
PS2, Line 279: d.
> nit: "purging" isn't a well-defined concept in Kudu, and so coming back to
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@285
PS2, Line 285: isPurgeTable
> nit: Would it make sense to call this "isSynchronizedTable"? We already use
This is slightly more generic than checking for synchronized because it doesn't check for "external". I did this in case Hive chooses to use the purge property for other things in the future. This function just indicates tables that are purged, either via being managed or the property.
I added an isSynchronizedTable function.
http://gerrit.cloudera.org:8080/#/c/14179/2/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/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@20
PS2, Line 20: import static org.hamcrest.CoreMatchers.containsString;
> nit: not used?
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@233
PS2, Line 233: altering
> Did this test alter the table? What is the rationale behind the test?
I moved this more basic validation up from below.
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@265
PS2, Line 265:
> nit: why 3 lines of space here?
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@297
PS2, Line 297: a external type
> nit: "an external, synchronized table"?
It's actually not required that it's synchronized. This is just show its type can be changed.
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@319
PS2, Line 319: {
> Generally this is used to define a namespace so we can use the name `altere
Primarily this is used in this test to scope the alteredTable variable within the {}. Without them, variable names would need to be unique or we would need to reuse variables.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_catalog.cc@215
PS2, Line 215: // Remove the Kudu-specific field 'kudu.table_id'.
> Removing the managed table validation should not bring any issues, as it wi
Correct, this is always removing the id if it exists. If it doesn't, it's a no-op.
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.h@191
PS1, Line 191: // Returns true if the HMS table is a managed table or
: // if the HMS is an external tabl
> This should be reworded slightly to avoid ambiguity. As written it's not cl
Done
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.cc@413
PS1, Line 413: (table.tableType == hms::HmsClient::kExternalTable &&
> Nit: indent by another two chars so the first part of the condition is nice
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_client.cc@410
PS2, Line 410: string
> nit: could be a const ref.
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/hms_itest-base.cc
File src/kudu/integration-tests/hms_itest-base.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/hms_itest-base.cc@168
PS2, Line 168: RETURN_NOT_OK(hms_client_->GetTable(database_name, table_name, &table));
> nit: spacing.
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/hms_itest-base.cc@175
PS2, Line 175: We also disable the HMS plugin validation to allow a managed table to
: // be altered to an external table.
> nit: I was a confused at first because I thought we were both pretending to
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/master_hms-itest.cc@438
PS2, Line 438: NO_FATALS(CheckTableDoesNotExist("default", "e"));
> nit: spacing.
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3694
PS2, Line 3694: external table
> nit: external table with purge key to be true.
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3694
PS2, Line 3694: external table
> nit: or "external, synchronized table."
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3757
PS2, Line 3757: external table
> nit: external table with purge key to be true (need to be synchronized).
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3757
PS2, Line 3757: external table
> nit: or "external, synchronized table."
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3760
PS2, Line 3760: SchemaBuilder().Build()));
> We're missing the kExternalType here.
Done
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/tool_action_hms.cc@369
PS2, Line 369: external_hms_tables_by_id
> nit: maybe label this non_external_hms_tables_by_id?
Maybe unsynchronized_hms_tables_by_id? That's sort of wrong too since we don't handle all table types, only managed and external.
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 09 Sep 2019 14:35:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 1:
(6 comments)
Just passing through; Andrew/Hao should definitely take a look.
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG@12
PS1, Line 12: Further, in
: Hive 4, non-transactional Hive tables will get auto-migrated
: to external tables with `external.table.purge=true`.
So with this patch, HMS sync is compliant with Hive 3 but not with Hive 4? Because HMS sync still uses the managed table type for sync'ed Kudu tables?
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG@17
PS1, Line 17: syncronized
synchronized
Below too
http://gerrit.cloudera.org:8080/#/c/14179/1//COMMIT_MSG@26
PS1, Line 26: manged
managed
http://gerrit.cloudera.org:8080/#/c/14179/1/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/14179/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@187
PS1, Line 187: oldTable.getTableType()
Should use oldTableType here.
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.h@191
PS1, Line 191: // Returns true if the HMS table is a managed table or an external table with
: // `external.table.purge = true`.
This should be reworded slightly to avoid ambiguity. As written it's not clear if the necessary condition is:
(MANAGED || EXTERNAL) && PURGE
Or:
MANAGED || (EXTERNAL && PURGE)
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/14179/1/src/kudu/hms/hms_client.cc@413
PS1, Line 413: (table.tableType == hms::HmsClient::kExternalTable && boost::iequals(externalPurge, "true"));
Nit: indent by another two chars so the first part of the condition is nicely aligned with the condition on the previous line.
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
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: Thu, 05 Sep 2019 18:47:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Greg Solovyev, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14179
to look at the new patch set (#4).
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 237 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/4
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Greg Solovyev, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14179
to look at the new patch set (#7).
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type. The HMS plugin
tests were also adjusted to use external purged tables by default to avoid
testing issues one the auto-migration code is enabled in Hive.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 281 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/7
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 Sep 2019 23:08:40 +0000
Gerrit-HasComments: No
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Greg Solovyev, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14179
to look at the new patch set (#5).
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type. The HMS plugin
tests were also adjusted to use external purged tables by default to avoid
testing issues one the auto-migration code is enabled in Hive.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 264 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/5
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 2:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/14179/2/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/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@183
PS2, Line 183: // The Kudu master is allowed to make these changes if necessary as it is a trusted user.
Is this in preparation for introducing tooling to migrate managed-->external+synchronized tables?
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@194
PS2, Line 194: !TableType.MANAGED_TABLE.name().equals(oldTableType) &&
: !(isExternalTable(oldTable) &&
Isn't some or all of this taken care of by isPurgeTable?
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@279
PS2, Line 279: d.
nit: "purging" isn't a well-defined concept in Kudu, and so coming back to this later on, it might not be immediately clear what this is saying. Mind elaborating on this in Kudu terms? e.g. something along the lines of:
Checks whether the table should be purged when deleted, i.e. the underlying Kudu table should be deleted when the HMS table entry is deleted.
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@285
PS2, Line 285: isPurgeTable
nit: Would it make sense to call this "isSynchronizedTable"? We already use "synchronized table" here and there, and I would appreciate keeping the table type naming (managed/synchronized/purge/etc) limited and consistent where we possible.
http://gerrit.cloudera.org:8080/#/c/14179/2/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/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@20
PS2, Line 20: import static org.hamcrest.CoreMatchers.containsString;
nit: not used?
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@297
PS2, Line 297: a external type
nit: "an external, synchronized table"?
http://gerrit.cloudera.org:8080/#/c/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@319
PS2, Line 319: {
> what's the point of {} here and above where there is no try-catch?
Generally this is used to define a namespace so we can use the name `alteredTable` in multiple "cases" within a single test case.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_client.cc@410
PS2, Line 410: string
nit: could be a const ref.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/hms_itest-base.cc
File src/kudu/integration-tests/hms_itest-base.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/hms_itest-base.cc@168
PS2, Line 168: RETURN_NOT_OK(hms_client_->GetTable(database_name, table_name, &table));
nit: spacing.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/hms_itest-base.cc@175
PS2, Line 175: We also disable the HMS plugin validation to allow a managed table to
: // be altered to an external table.
nit: I was a confused at first because I thought we were both pretending to be the master _and_ doing something extra to the HMS plugin to disable validation. Maybe remove this second piece?
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/master_hms-itest.cc@257
PS2, Line 257: then drop it from the HMS, and ensure
: // the Kudu table is deleted.
"...then alter it in the HMS and ensure the Kudu table is actually altered." ?
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/integration-tests/master_hms-itest.cc@438
PS2, Line 438: NO_FATALS(CheckTableDoesNotExist("default", "e"));
nit: spacing.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3694
PS2, Line 3694: external table
> nit: external table with purge key to be true.
nit: or "external, synchronized table."
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3757
PS2, Line 3757: external table
> nit: external table with purge key to be true (need to be synchronized).
nit: or "external, synchronized table."
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3760
PS2, Line 3760: SchemaBuilder().Build()));
We're missing the kExternalType here.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/tool_action_hms.cc@369
PS2, Line 369: external_hms_tables_by_id
nit: maybe label this non_external_hms_tables_by_id?
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 08 Sep 2019 04:41:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Greg Solovyev, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14179
to look at the new patch set (#3).
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
---
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.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
11 files changed, 237 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/14179/3
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14179/4/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/14179/4/src/kudu/hms/hms_catalog.cc@189
PS4, Line 189: // If this is an external table, only upgrade the storage handler.
: if (table.tableType == HmsClient::kExternalTable) {
: table.parameters[HmsClient::kStorageHandlerKey] = HmsClient::kKuduStorageHandler;
: }
Sorry missed this in last review, do we need to check for external.table.purge key here as well?
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 09 Sep 2019 19:02:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 09 Sep 2019 17:18:21 +0000
Gerrit-HasComments: No
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Removed -Verified by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14179/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14179/2//COMMIT_MSG@13
PS2, Line 13: Hive tables
Does it matter if they are managed or external originally?
http://gerrit.cloudera.org:8080/#/c/14179/2/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/14179/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@233
PS2, Line 233: altering
Did this test alter the table? What is the rationale behind the test?
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/hms/hms_catalog.cc@215
PS2, Line 215: // Remove the Kudu-specific field 'kudu.table_id'.
Removing the managed table validation should not bring any issues, as it will be a no-op for tables that has not id, right?
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3694
PS2, Line 3694: external table
nit: external table with purge key to be true.
http://gerrit.cloudera.org:8080/#/c/14179/2/src/kudu/tools/kudu-tool-test.cc@3757
PS2, Line 3757: external table
nit: external table with purge key to be true (need to be synchronized).
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 06 Sep 2019 17:51:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14179/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14179/6/src/kudu/tools/kudu-tool-test.cc@3775
PS6, Line 3775: // Test case: legacy managed table.
> Sorry that I should mention in the previous comment, can you also add a tes
I am not sure a legacy external table with a purge key could actually happen give the new format was defined and used pre Hive 3. But we should test it just in case anyway. Will add one.
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 Sep 2019 22:11:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
[hms] Sync external HMS tables with purge property
This patch is another patch in preparation for Hive 3 and 4.
In Hive 3 the meaning of managed tables is a bit more strict.
To replace the old behavior of a managed table, external tables
now have the `external.table.purge` property. Further, in
Hive 4, non-transactional Hive tables will get auto-migrated
to external tables with `external.table.purge=true`.
With this change, the HMS sync now treats managed tables and
external tables with `external.table.purge=true` as synchronized tables.
External tables with `external.table.purge=false` will continue to be
ingnored like all external tables before.
To avoid orphaned tables, the HMS plugin was changed to restrict changing
the `external.table.purge` property. This is similar to the existing
functionality which prevented changing the table type. The HMS plugin
tests were also adjusted to use external purged tables by default to avoid
testing issues one the auto-migration code is enabled in Hive.
Last, the HMS tooling was updated to differentiate between synchronized
and unsynchronized tables instead of explicitly managed and external tables.
Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Reviewed-on: http://gerrit.cloudera.org:8080/14179
Reviewed-by: Hao Hao <ha...@cloudera.com>
Tested-by: Grant Henke <gr...@apache.org>
---
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.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
11 files changed, 281 insertions(+), 71 deletions(-)
Approvals:
Hao Hao: Looks good to me, approved
Grant Henke: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 Sep 2019 22:29:01 +0000
Gerrit-HasComments: No
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [hms] Sync external HMS tables with purge property
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14179 )
Change subject: [hms] Sync external HMS tables with purge property
......................................................................
Patch Set 6:
(1 comment)
LGTM, just one more test case to have better coverage.
http://gerrit.cloudera.org:8080/#/c/14179/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14179/6/src/kudu/tools/kudu-tool-test.cc@3775
PS6, Line 3775: // Test case: legacy managed table.
Sorry that I should mention in the previous comment, can you also add a test case for legacy external table with purge key?
--
To view, visit http://gerrit.cloudera.org:8080/14179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I942515c015b1a4a63cf08383c331c4e19350c312
Gerrit-Change-Number: 14179
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 Sep 2019 19:59:42 +0000
Gerrit-HasComments: Yes