Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17298 )
Change subject: IMPALA-10648: Invalidate catalogd table cache for hms ddl apis which modify tables and partitions. ...................................................................... Patch Set 4: (26 comments) http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@141 PS3, Line 141: import org.apache.hadoop.hive.metastore.api.LockRequest; > Is this used somewhere? No. Will remove it. Thanks for pointing it out. http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@222 PS3, Line 222: UnlockRequest; > Is this used somewhere? Will remove it. http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@347 PS3, Line 347: "is > nit: We use 4 spaces indention for multi-line statement. However, many chan Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@348 PS3, Line 348: } > line too long (91 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@349 PS3, Line 349: > nit: 4 spaces indention Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@890 PS3, Line 890: client.getHiveClient().getThriftClient().add_partition(partition); > line too long (99 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@891 PS3, Line 891: invalidateNonTransactionalTableIfExists(partition.getDbName(), > line too long (94 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@904 PS3, Line 904: invalidateNonTransactionalTableIfExists(partition.getDbName(), > line too long (94 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@914 PS3, Line 914: try (MetaStoreClient client = catalog_.getMetaStoreClient()) { > line too long (102 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@915 PS3, Line 915: > Is it possible that these partitions belong to different tables? Very likely the partitions belong to the same table. I will confirm and make the change accordingly. Thanks for pointing it out. http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@929 PS3, Line 929: int numPartitionsAdded = client.getHiveCl > Same question as above Will check this one too. http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@930 PS3, Line 930: .getThriftClient().add_partitions_pspec(list); > line too long (104 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@955 PS3, Line 955: AddPartitionsResult result = client.getHiveClient().getThriftClient() > line too long (114 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@968 PS3, Line 968: try (MetaStoreClient client = catalog_.getMetaStoreClient()) { > line too long (104 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@994 PS3, Line 994: throws InvalidObjectException, AlreadyExistsException, MetaException, TException { > line too long (91 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1021 PS3, Line 1021: throws NoSuchObjectException, MetaException, TException { > line too long (94 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1034 PS3, Line 1034: boolean deleteData) > line too long (106 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1049 PS3, Line 1049: String partName, boolean deleteData, EnvironmentContext envContext) > line too long (91 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1086 PS3, Line 1086: String d > nit: 4 spaces indention Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1087 PS3, Line 1087: throws TException > nit: 8 spaces indention and move the comma above Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1102 PS3, Line 1102: String destinationTableName) > line too long (104 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1457 PS3, Line 1457: } > line too long (100 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1458 PS3, Line 1458: > line too long (108 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2846 PS3, Line 2846: * if hms ddl apis are accessed from catalogd's metastore server. > line too long (115 > 90) Ack http://gerrit.cloudera.org:8080/#/c/17298/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2849 PS3, Line 2849: o > nit: In our code style, we need one space after "if". Many changes in this Thanks for pointing it out. Will fix it. http://gerrit.cloudera.org:8080/#/c/17298/3/tests/custom_cluster/test_metastore_service.py File tests/custom_cluster/test_metastore_service.py: http://gerrit.cloudera.org:8080/#/c/17298/3/tests/custom_cluster/test_metastore_service.py@424 PS3, Line 424: ) > nit: remove this blank line Ack -- To view, visit http://gerrit.cloudera.org:8080/17298 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb9cc22ebfb51948433e4d57f4705ce201acaf98 Gerrit-Change-Number: 17298 Gerrit-PatchSet: 4 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Comment-Date: Wed, 14 Apr 2021 23:16:27 +0000 Gerrit-HasComments: Yes
