Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 )
Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled ...................................................................... Patch Set 4: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/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/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@131 PS4, Line 131: below doesn't fail. nit: If the master addresses property isn't set, won't kuduSyncEnabled() just return false rather than fail? Nevermind. Looking around, if there is no master addresses when creating a table, we should be throwing an exception, thereby signaling to the HMS that the action is rejected. kuduSyncEnabled() is necessarily permissive because in most other cases we expect that a missing masters field refers to a non-Kudu or non-synchronized table. Could you update this to clarify that the failure isn't an exception, but rather being too permissive? http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@179 PS4, Line 179: // Dropping tables from Hive is okay and Kudu will be notified via the : // notification listener. nit: I didn't understand the context of this comment until I read the gerrit discussion that spurred it. Could you add a bit more detail, e.g. "Drop table requests that don't come from the Kudu master may not set the table ID, e.g. when dropping tables via Hive, or dropping orphaned HMS entries. Such tables are dropped in Kudu by name via the notification listener" -- 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: 4 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: Wed, 09 Sep 2020 18:29:34 +0000 Gerrit-HasComments: Yes
