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 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/13317/5/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/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@141 PS5, Line 141: // potential schema alterations are coming from the Kudu master > nit: add period Done http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@145 PS5, Line 145: } else { > nit: the 'else' clause is not needed because of 'return' above ? Done http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@154 PS5, Line 154: // schema alterations are coming from the Kudu master > nit: add period Done http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@155 PS5, Line 155: checkNoKuduProperties(newTable); > I don't have enough context on this, but this looks a bit strange: no Kudu Yeah, this is correct. Because checkNoKuduProperties verify that legacy tables don't use the new storage handler and don't contain table_ID property. We have test at L258 at TestKuduMetastorePlugin. http://gerrit.cloudera.org:8080/#/c/13317/5/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/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@258 PS5, Line 258: // Check that altering table with Kudu storage handler to legacy format : // succeeds. : { : Table alteredTable = table.deepCopy(); : alteredTable.getParameters().clear(); : alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE, : KuduMetastorePlugin.LEGACY_KUDU_STORAGE_HANDLER); : alteredTable.putToParameters(KuduMetastorePlugin.LEGACY_KUDU_TABLE_NAME, : "legacy_table"); : alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY, : "localhost"); : client.alter_table(table.getDbName(), table.getTableName(), alteredTable); : } > I thought that after recent developments this should no longer be a case, n Even with recent development which is to always use new storage handler going forward (>= Kudu 1.10 and >= Impala 3.3). We still should allow table to be downgraded to use legacy storage handlers, for cases when users want to downgrade to older versions( <Impala 3.3). http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@331 PS5, Line 331: table > nit: table's Done http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@338 PS5, Line 338: table > nit: table's 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: 5 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: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 17 May 2019 22:57:28 +0000 Gerrit-HasComments: Yes
