Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/17244 )
Change subject: IMPALA-10613: Standup HMS thrift server in Catalog ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/17244/4/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/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@91 PS4, Line 91: FALLBACK_FILE_MD_TIME_WARN_THRESHOLD nit: usually we append _MS if the time units is ms. It is done for the external config options but it is a good convention for internal constants as well. http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@91 PS4, Line 91: 100 I am curious about this value..should this depend on the number of files for which the metadata needs to be loaded ? http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@126 PS4, Line 126: checkCondition(getTableRequest.getEngine() != null, "Column stats are requested " The comment above says to check the processor engine is set to Impala but this is only checking that engine is non-null. http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@135 PS4, Line 135: if (getTableRequest.isSetValidWriteIdList()) { For transactional tables, should we have a precondition that the ValidWriteIdList is non-null ? http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@205 PS4, Line 205: * @param catalog The catalog instance which caches the table. nit: the defaultCatalog is missing in the params. http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogHmsClientUtils.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogHmsClientUtils.java: http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogHmsClientUtils.java@50 PS4, Line 50: the nit: remove 'the' http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java: http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@57 PS4, Line 57: //TODO add It says TODO here but if --enable_catalogd_hms_cache is true, this patch does serve certain requests from the cache, right ? http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@250 PS4, Line 250: metaserver nit: At other places you have used 'metastore server' or 'metastore service'. Would be good to use the same here. http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@146 PS4, Line 146: return super.get_partitions_by_names_req(getPartitionsByNamesRequest); This doesn't do any logging of the request (unlike the get_partition_by_expr) ? Would be good to have the INFO log entry. -- 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: 4 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: Tue, 06 Apr 2021 00:53:15 +0000 Gerrit-HasComments: Yes
