Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 )
Change subject: WIP - KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/16388/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/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@128 PS1, Line 128: // In the case of table cre > +1 We need to be sure the master addresses property is set regardless of sync. I will break out that check and move the rest below. http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@128 PS1, Line 128: // In the case of table cre > Shouldn't we do this after checking whether HMS sync is enabled? Done http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@164 PS1, Line 164: (!isK > Should we call checkNoKuduProperties here as well? I don't think we need to, this is drop table so it's not introducing properties. http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@172 PS1, Line 172: // Only validate tables for clusters with HMS sync enabled. : if (!kuduSyncEnabled(table)) { : return; : } > This case indicates that the table is a Kudu table but it does not has a ta This is the case when the drop table request doesn't have target table id in its context. I think we want to support dropping Kudu tables so this is valid. This case is also needed in order to support dropping orphan HMS tables. I will add a comment. http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@201 PS1, Line 201: Table oldTable = tableEvent.getOldTable(); > checkNoKuduProperties does not check for KUDU_MASTER_ADDRS_KEY - is that in I think it's because the Legacy Kudu tables can also have that property. I am not sure it's worth changing. If other tables had it it would not interfere with HMS sync functionality like the table id would. http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@201 PS1, Line 201: ble oldTable = tableE > another comment on this is that checkNoKuduProperties will also call isKudu It is a bit redundant but given the other usages and the somewhat complicated logic in this class I am not sure I want to change it. http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@206 PS1, Line 206: Allow non-Kudu > Thinking about the execution path for non-Kudu tables, it will return faste This check requires a remote RPC to the Kudu master. The one above only checks the tableEvent contents. I don't want requests to Kudu for non-kudu tables. http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@327 PS1, Line 327: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@472 PS1, Line 472: etHiveMetastoreCo > Do we need to do anything special, pass any configs, etc., in case this is I will test this manually to be sure. I am hoping we get it for free. http://gerrit.cloudera.org:8080/#/c/16388/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/16388/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@57 PS1, Line 57: private EnvironmentContext masterContext() { > Remove this import since you've removed the annotation below? Done -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Sep 2020 18:25:43 +0000 Gerrit-HasComments: Yes
