Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 )
Change subject: IMPALA-9824: MetastoreClientPool should be singleton ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/16030/3/common/thrift/BackendGflags.thrift File common/thrift/BackendGflags.thrift: http://gerrit.cloudera.org:8080/#/c/16030/3/common/thrift/BackendGflags.thrift@161 PS3, Line 161: 69 > Should this be 68 and the next one 69? Thanks for pointing this out. Fixed it. http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@89 PS3, Line 89: protected final CatalogObjectCache<DataSource> dataSources_ = new CatalogObjectCache<>(); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/EmbeddedMetastoreClientPool.java File fe/src/main/java/org/apache/impala/catalog/EmbeddedMetastoreClientPool.java: http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/EmbeddedMetastoreClientPool.java@36 PS3, Line 36: > nit: duplicate spaces Done http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java File fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java: http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@20 PS1, Line 20: import java.nio.file.Path; > nit: could you move this to line 36 with "com.google" package group? Done http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@167 PS1, Line 167: */ : public static synchronized MetaStoreClientPool get() { : i > Is it intended to do this twice? Thanks for pointing this out. Removed it. http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@178 PS1, Line 178: > nit: could you remove this empty return comment? Done http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java File fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java: http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@190 PS3, Line 190: cfg.is_coordinator > This doesn't look like the identification of a coordinator. The catalogd fl Thanks for pointing this out. I didn't realize that this configuration defaults to true. I introduced a new configuration for is_catalog which catalogd-main.cc sets to true. http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@191 PS3, Line 191: pool_ = > nit: don't need to update pool_ here since it's updated in get(). thanks for catching this. Fixed it. http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@203 PS3, Line 203: unit > nit: duplicate "unit" Done http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@229 PS3, Line 229: > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@238 PS3, Line 238: BackendConfig.INSTANCE.getBackendCfg() > nit: can use "cfg" directly Done http://gerrit.cloudera.org:8080/#/c/16030/3/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@258 PS3, Line 258: //TODO (Vihang) why do we need a timeout of 0? > Can we simply use initial_hms_cnxn_timeout_s? I thought of that but I was not sure of the reasoning of the original change which had it set to 0. After reading more on how this config is used, a value of 0 is not particularly harmful since RetryingMetastoreClient has built-in retries. I am not sure what is the motivation of having such a configuration in the first place since RetryingMetastoreClient has a sufficiently long retry interval. -- To view, visit http://gerrit.cloudera.org:8080/16030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f Gerrit-Change-Number: 16030 Gerrit-PatchSet: 3 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 10 Jun 2020 18:04:39 +0000 Gerrit-HasComments: Yes
