Grant Henke 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: (18 comments) http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG@12 PS3, Line 12: metdata > typo: metadata Done http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG@13 PS3, Line 13: syncronization > synchronization Done http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG@13 PS3, Line 13: syncronization > typo: synchronization Done 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@238 PS3, Line 238: a > nit: an ? Done http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@424 PS3, Line 424: to the relevant metadata > Is it possible that storage handler changes between 'before' and 'after' ta yes, which is why I check for it below (hive_metastoreConstants.META_TABLE_STORAGE). http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@431 PS3, Line 431: kuduMetadataUnchanged > Does it make sense to add a consistency check into the body of kuduMetadata It feels a bit verbose. It's not technically required for the method to work and we do already validate that in its usage. http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@431 PS3, Line 431: public > Is it really necessary to declare this method as public given the presence I can make it package protected. 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 Done 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: 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))) { > nit: to reduce the clutter, maybe introduce 'paramsBefore' and 'paramsAfter Done 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 These don't technically get "synced" with Kudu but they would change the perceived synchronization state (whether or not a table is considered synchronized & in-sync by Kudu) so we should check if HMS sync is enabled to ensure that is allowed. 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 com For a string the equals() method is used to compare the contents of the string and not the reference. Objects.equals() with a String object will use the String equals() method. I use Objects.equals() so I don't need to handle nulls manually. 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: ; > nit: . Done 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 Done 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 Done 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: change > nit: changed ? Done http://gerrit.cloudera.org:8080/#/c/17531/3/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/17531/3/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@652 PS3, Line 652: manged > managed Done http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@716 PS3, Line 716: before, after > Does it make sense to add a reverse case to test the removal of a column as Done http://gerrit.cloudera.org:8080/#/c/17531/3/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@719 PS3, Line 719: a column > nit: a column comment ? Done -- 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: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 02 Jun 2021 20:40:39 +0000 Gerrit-HasComments: Yes
