Andrew Wong 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 1: (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. 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 : * to the specified Kudu table. This provides some amount of concurrency safety, : * so that the Kudu Master can ensure it is operating on the correct table entry. Can you update this to describe how this plugin handles the legacy storage handler? 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: // Allow non-Kudu tables (even the legacy ones) to be altered. This should probably be updated. Without further clarification, the new code makes everything below L138 more difficult to reason about IMO. Previously, after L138 we were guaranteed that the old table was either a Kudu table or a Legacy table. Now it's less obvious what guarantees there are. 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: if (!isLegacyKuduTable(newTable)) { Why isn't this using isKuduTable()? Why !isLegacyKuduTable()? Does it matter? Same below. 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.ArrayList; : import java.util.List; Not used? 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: table.getDbName(), table.getTableName() nit: perhaps less confusing as alteredTabled.getDbName() and alteredTable.getTableName()? 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: altering nit: renaming 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: client.dropTable(table.getDbName(), table.getTableName()); Should this be dropping the renamed table? Alternatively, put the above new cases into their own separate test case to test doing things with legacy tables. As it stands, it's kind of unclear to me which test cases are testing legacy metadata. If you do, you could also put L343-L347 in the same test case. -- 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: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 13 May 2019 07:13:24 +0000 Gerrit-HasComments: Yes
