Alexey Serbin 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: (12 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 http://gerrit.cloudera.org:8080/#/c/17531/3//COMMIT_MSG@13 PS3, Line 13: syncronization typo: 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@238 PS3, Line 238: a nit: an ? 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' table's states? 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 of @VisibleForTesting annotation? 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 kuduMetadataUnchanged() to make sure both before and after are Kudu tables? 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' variables here (even maybe name them 'pb' and 'pa' if Java coding style can tolerate that heresy)? 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: . 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 ? 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 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 well? 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 ? -- 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:06:48 +0000 Gerrit-HasComments: Yes
