Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17531 )
Change subject: [hms] Avoid blocking table alterations that are not relevant to Kudu ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG@13 PS3, Line 13: syncronization synchronization http://gerrit.cloudera.org:8080/#/c/17531/3/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/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@433 PS3, Line 433: before.getParameters() nit: maybe pull before.getParameters() and after.getParameters() out into a local variable? http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@432 PS3, Line 432: // If any of the Kudu table properties have changed, return false. : if (!Objects.equals(before.getParameters().get(hive_metastoreConstants.META_TABLE_STORAGE), : after.getParameters().get(hive_metastoreConstants.META_TABLE_STORAGE)) || : !Objects.equals(before.getParameters().get(KUDU_MASTER_ADDRS_KEY), : after.getParameters().get(KUDU_MASTER_ADDRS_KEY)) || : !Objects.equals(before.getParameters().get(KUDU_TABLE_ID_KEY), : after.getParameters().get(KUDU_TABLE_ID_KEY)) || : !Objects.equals(before.getParameters().get(KUDU_TABLE_NAME_KEY), : after.getParameters().get(KUDU_TABLE_NAME_KEY)) || : !Objects.equals(before.getParameters().get(KUDU_CLUSTER_ID_KEY), : after.getParameters().get(KUDU_CLUSTER_ID_KEY))) { : return false; : } I'm a little surprised to see some fields here that don't get synchronized with Kudu, like META_TABLE_STORAGE and KUDU_MASTER_ADDRS_KEY. If these do change, is it actually necessary that Kudu is online for us to check HMS sync status? http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@454 PS3, Line 454: !Objects.equals(before.getDbName(), after.getDbName()) I'm kind of surprised that we're doing Object.equals() and not a string comparison. Under the hood is Hive keeping the same objects stored for the identical values in Table? http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@462 PS3, Line 462: e; nit: period http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@462 PS3, Line 462: e nit: changed -- To view, visit http://gerrit.cloudera.org:8080/17531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2517d891ab7168164700bb3ae642c49bde54b9db Gerrit-Change-Number: 17531 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 02 Jun 2021 20:10:04 +0000 Gerrit-HasComments: Yes
