Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17244 )
Change subject: IMPALA-10613 : Standup HMS thrift server in Catalog ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java File fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java: http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@44 PS3, Line 44: import org.apache.hadoop.hive.common.ValidReaderWriteIdList; nit: unused import http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@141 PS3, Line 141: deepCopy Could you add a comment for deepCopy() here? We already deep copy the hms table object in Table.getPartialInfo(): https://github.com/apache/impala/blob/22d1f8c7fe14e636d9186f69cf156851879f37b3/fe/src/main/java/org/apache/impala/catalog/Table.java#L661 Maybe we don't need this? http://gerrit.cloudera.org:8080/#/c/17244/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/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@141 PS3, Line 141: import org.apache.hadoop.hive.metastore.api.InvalidPartitionException; nit: unused import http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@222 PS3, Line 222: import org.apache.hadoop.hive.metastore.api.UnknownPartitionException; nit: unused import http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@264 PS3, Line 264: import org.apache.impala.catalog.CatalogException; nit: unused import http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@751 PS3, Line 751: info nit: Will this too verbose? Consider "trace"? http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@759 PS3, Line 759: Table tbl = result.getTable(); : boolean isTransactional = tbl.getParameters() != null && AcidUtils : .isTransactionalTable(tbl.getParameters()); nit: move these after the following if-clause http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@778 PS3, Line 778: setFileMetadata At the first glance, I thought this is setting the file metadata using those cached in catalogd. However, it always triggers file metadata loading. Can we rename this to "loadFileMetadata" or "loadAndSetFileMetadata"? http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1256 PS3, Line 1256: If the hive-exec jar is not present in the classpath, we fall-back to HMS since : * Catalog has no way to deserialize the expression sent over by the client. It seems we just forward the call. Are we missing some codes here? EDIT: Seems copied from CatalogMetastoreServiceHandler.get_partitions_by_expr(). Maybe we can remove these comments. http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1300 PS3, Line 1300: if (fallBackToHMSOnErrors_) { : return; : } nit: Many simple if-clauses in this patch are not in our code style. I think we prefer if (fallBackToHMSOnErrors_) return; It'd be better to keep the same code style. But if these codes are copied from Hive, please ignore this. -- To view, visit http://gerrit.cloudera.org:8080/17244 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b306f91d63cb5137c178e8e72b6e8b578a907b5 Gerrit-Change-Number: 17244 Gerrit-PatchSet: 3 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Mon, 05 Apr 2021 10:14:14 +0000 Gerrit-HasComments: Yes
