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 4: (3 comments) Thanks for the double-refactor; just some nits at this point. http://gerrit.cloudera.org:8080/#/c/13317/4/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/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@144 PS4, Line 144: } else { : // Allow legacy tables to be altered without introducing Kudu-specific : // properties. : checkNoKuduProperties(newTable); : } nit: for parity with the L156, could write this as ... checkNoAlterKuduSchema(tableEvent, oldTable, newTable); return; } checkNoKuduProperties(newTable); } or change L156 to use an `else` http://gerrit.cloudera.org:8080/#/c/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@158 PS4, Line 158: properties. And the : // schema alteration is not coming through Hive, if any. nit: reword a bit: "properties, and that any potential schema alterations are coming from the Kudu master." Same above in both comments. http://gerrit.cloudera.org:8080/#/c/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@240 PS4, Line 240: * Checks that the Kudu table schema should not be altered through Hive. An Impala user might be the one updating the HMS metadata though, no? Would this be more accurate as: "Checks that the table schema can only be altered by an action from the Kudu Master." ? If so, perhaps update the exception text so that's clear as well. By the same token, would something like checkOnlyKuduMasterCanAlterSchema be an apt name? -- 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: 4 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: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 15 May 2019 07:40:22 +0000 Gerrit-HasComments: Yes
