Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 )
Change subject: IMPALA-9824: MetastoreClientPool should be singleton ...................................................................... Patch Set 3: (8 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? 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 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 flags can still set this to true while it's previously ignored. I wonder if there are better ways to detect this. 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(). 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" 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 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 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? -- 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 13:41:37 +0000 Gerrit-HasComments: Yes
