Vihang Karajgaonkar 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: (30 comments) http://gerrit.cloudera.org:8080/#/c/17244/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17244/3//COMMIT_MSG@7 PS3, Line 7: > nit: no whitespace Done http://gerrit.cloudera.org:8080/#/c/17244/3/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/17244/3/be/src/catalog/catalog-server.cc@94 PS3, Line 94: hms_port_number > hms_port Done http://gerrit.cloudera.org:8080/#/c/17244/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/17244/3/be/src/common/global-flags.cc@352 PS3, Line 352: " it will be redirected to HMS."); > nit: add a blank line here Done 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 Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@87 PS3, Line 87: CatalogHMSAPIHelper > nit: I think our naming rules prefer "CatalogHmsApiHelper" Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@96 PS3, Line 96: maxNonHdfsPartsParallelLoad > Should this be maxHdfsPartsParallelLoad(), or we just choose this because i Yes, I thought of just reusing the larger size of the 2 pools since this is a static pool which is shared by all the API invocations in case of cache misses. I have a TODO here to have it separately configurable. Perhaps I should just use a constant of 20 to avoid confusion. Any thoughts? http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@121 PS3, Line 121: GetPartialCatalogObjectRequestBuilder reqBuilder = new GetPartialCatalogObjectRequestBuilder() > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@138 PS3, Line 138: CDPD-14901 > File an upstream JIRA for this? Thanks for spotting this. Updated the comment with the latest observations about this. 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 t Thanks for catching this. Yes, it doesn't look like this is necessary. I was making a copy a bit pessimistically since that would mean that we are using implementation knowledge of the API. The copy was necessary since the table was being modified later down below. I added a comment explaining the same but I would be okay to remove the deepCopy too if you think that is an overkill. http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@225 PS3, Line 225: GetPartialCatalogObjectRequestBuilder catalogReq = new GetPartialCatalogObjectRequestBuilder() > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@519 PS3, Line 519: + "fallback path. Time taken: {} msec", getPartsResult.getPartitionsSize(), > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/CatalogHMSAPIHelper.java@523 PS3, Line 523: + "fallback path. Time taken: {} msec", getPartsResult.getPartitionsSize(), > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/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/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogHMSClientUtils.java@46 PS3, Line 46: CatalogHMSClientUtils > nit: CatalogHmsClientUtils? Done 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 My IDE tells me that this is not a unused import. Some of the methods below throw this exception in their signature. 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 Done 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 Done 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"? Ideally, this would be logged only when the table doesn't exist in the catalogd's cache. I found it useful for debugging. I think debug level is more appropriate than 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 Not sure I understand this comment. The following 2 if clauses are not nested and the first one requires the tbl variable and the second on requires isTransactional. May be you meant to move isTransactional only? 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 thos Done. http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1237 PS3, Line 1237: public GetPartitionsPsWithAuthResponse get_partitions_ps_with_auth_req(GetPartitionsPsWithAuthRequest req) > line too long (107 > 90) Done 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? Yes, the comment was removed. Thanks! 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 thin Yeah, my IDE keeps reformatting them. I have modified a few to conform to our code style. http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1632 PS3, Line 1632: throws NoSuchObjectException, InvalidObjectException, MetaException, InvalidInputException, TException { > line too long (110 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1643 PS3, Line 1643: throws NoSuchObjectException, MetaException, InvalidObjectException, InvalidInputException, TException { > line too long (110 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1654 PS3, Line 1654: throws NoSuchObjectException, MetaException, InvalidObjectException, InvalidInputException, TException { > line too long (110 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1664 PS3, Line 1664: throws AlreadyExistsException, InvalidObjectException, MetaException, NoSuchObjectException, TException { > line too long (111 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2435 PS3, Line 2435: public WMCreateOrDropTriggerToPoolMappingResponse create_or_drop_wm_trigger_to_pool_mapping( > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHMSFileMetadataTest.java File fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHMSFileMetadataTest.java: http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHMSFileMetadataTest.java@50 PS3, Line 50: CatalogHMSFileMetadataTest > nit: CatalogHmsFileMetadataTest? Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/test/java/org/apache/impala/catalog/metastore/EnableCatalogdHMSCacheFlagTest.java File fe/src/test/java/org/apache/impala/catalog/metastore/EnableCatalogdHMSCacheFlagTest.java: http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/test/java/org/apache/impala/catalog/metastore/EnableCatalogdHMSCacheFlagTest.java@50 PS3, Line 50: EnableCatalogdHMSCacheFlagTest > nit: EnableCatalogdHmsCacheFlagTest? Done http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java File fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java: http://gerrit.cloudera.org:8080/#/c/17244/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@67 PS3, Line 67: startHMS > nit: startHms? Done -- 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 21:49:46 +0000 Gerrit-HasComments: Yes
