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 4: (13 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@141 PS3, Line 141: > Thanks for catching this. Yes, it doesn't look like this is necessary. I wa I think this won't have significant impact on performance. But if we do so, it makes sense to do the same for partitions in getPartitionsByNames() as well. Deep copying all partitions could impact performance for large tables. 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@225 PS4, Line 225: // TODO add file-metadata to Partitions nit: move this TODO to line 275. Is the common pattern is using this method to get the filtered partition names, and then use getPartitionsByNames to get he file metadata? Or are there other use cases that we don't need the file-metadata? http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@299 PS4, Line 299: if (condition) { : return; : } nit: one line http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@312 PS4, Line 312: if (catalogName == null) { : return; : } nit: one line http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@365 PS4, Line 365: //TODO add table id here when client passes it (CDPD-14901) Need to update this as well. http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@414 PS4, Line 414: if (networkAddresses.isEmpty()) { : return result; : } nit: one line http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@491 PS4, Line 491: if (getPartsResult.getPartitionsSize() == 0) { : return; : } nit: one line http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3318 PS4, Line 3318: } nit: keep the blank line 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@105 PS4, Line 105: public void preServe() { : : } nit: one line http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@182 PS4, Line 182: TODO Add SASL and ssl support File an upstream JIRA for this? http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@205 PS4, Line 205: //TODO add config for this? : boolean useCompactProtocol = false; Will setting this to true help on large tables? Do you plan to file an upstream JIRA? http://gerrit.cloudera.org:8080/#/c/17244/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServer.java@265 PS4, Line 265: * @return nit: remove 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.LockRequest; > My IDE tells me that this is not a unused import. Some of the methods below hmm, I simply search "InvalidPartitionException" but can't find one... -- 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 08:40:09 +0000 Gerrit-HasComments: Yes
