Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/13317/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/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@49 PS1, Line 49: * The plugin enforces that Kudu table entries in the HMS always contain : * two properties: a Kudu table ID and the Kudu master addresses. It also : * enforces that non-Kudu tables do not have these properties (except cases : * when upgrading tables with legacy Kudu storage handler to be Kudu tables : * or downgrading from the other way around). The plugin considers entries : * to be Kudu tables if they contain the Kudu storage handler. : * : * Additionally, the plugin checks that when particular events have an : * environment containing a Kudu table ID, that event only applies > Can you update this to describe how this plugin handles the legacy storage Done http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@133 PS1, Line 133: > This should probably be updated. Without further clarification, the new cod Done http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@143 PS1, Line 143: return; > Why isn't this using isKuduTable()? Why !isLegacyKuduTable()? Does it matte It is important to prevent cases alter Kudu table to be without Kudu storage handler. However for the below it should be equivalent, as there is only KuduTable or LegacyTable at that point. http://gerrit.cloudera.org:8080/#/c/13317/1/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/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@25 PS1, Line 25: import java.util.UUID; : > Not used? Done http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@326 PS1, Line 326: cy Kudu table without context succeeds. > nit: perhaps less confusing as alteredTabled.getDbName() and alteredTable.g Not quite follow this. I think here it is intend to use the original table name. http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@329 PS1, Line 329: > nit: renaming Done http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@337 PS1, Line 337: // Check that renaming legacy table schema succeeds. > Should this be dropping the renamed table? Done -- To view, visit http://gerrit.cloudera.org:8080/13317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3 Gerrit-Change-Number: 13317 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 14 May 2019 00:55:13 +0000 Gerrit-HasComments: Yes
