Alexey Serbin 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 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 ? 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 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 properties in legacy Kudu table. Is it correct? Do we have a test scenario for this at least? 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, no? If such, should we modify the code of the handler and add a negative test scenario instead? 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 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 -- 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 <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: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 17 May 2019 03:06:51 +0000 Gerrit-HasComments: Yes
