[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Vihang Karajgaonkar has abandoned this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Abandoned Abandoning this patch based on my comment earlier. I don't think this is a critical issue currently and given that this can introduce some test flakiness, I would rather abandon this patch and revisit this later in the future if needed. -- 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: abandon Gerrit-Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f Gerrit-Change-Number: 16030 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 5: When ran the tests I realized that TestCaseLoaderTest fails when we run it along with other FE tests. The tests works if we run it alone. This issue is that during our tests we execute multiple mvn tests in the same JVM and hence the expectation that there must be one metastorePool per process fails. I think there is no good way to solve these two conflicting goals. I believe that metastore pool being tightly coupled with each instance of Catalog is a better model since that way we can have multiple Catalogs in the same processes (for testing or otherwise). Any thoughts Quanlong? -- 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: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 11 Jun 2020 23:58:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 5: Code-Review+2 (1 comment) 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_catalog) re > Thanks for pointing this out. I didn't realize that this configuration defa Thanks, that make sense to me. -- 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: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 11 Jun 2020 00:48:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6276/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Jun 2020 19:53:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Vihang Karajgaonkar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. IMPALA-9824: MetastoreClientPool should be singleton This patch is a refactor-only patch. It changes the MetastoreClientPool into process wide singleton object. Currently, it is possible to instantiate multiple MetastoreClientPools although it doesn't seem to be a problem since all the places which instantiate it happen to run in separate processes. It would be better to enforce this in code. Making MetastoreClientPool a singleton also makes it easier to access from anywhere in the code without the need to explicitly pass its reference around which complicates the code flow unnecessarily. Additionally, this patch also introduces two new configurations coordinator_initial_hms_connections and catalog_initial_hms_connections which define the initial number of HMS clients which are created for the respective services. The default values of both the configurations are kept same as previous values from the code. Testing: 1. No new tests were added since this was mostly a refactor-only patch. 2. Ran existing core tests. Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f --- M be/src/catalog/catalog.cc M be/src/catalog/catalogd-main.cc M be/src/service/frontend.cc M be/src/util/backend-gflag-util.cc M common/thrift/BackendGflags.thrift M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java R fe/src/main/java/org/apache/impala/catalog/EmbeddedMetastoreClientPool.java M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java M fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java M fe/src/test/java/org/apache/impala/common/AbstractFrontendTest.java M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java A fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestTransientCatalog.java M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java 19 files changed, 250 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/16030/5 -- 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: newpatchset Gerrit-Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f Gerrit-Change-Number: 16030 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
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 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
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Wed, 10 Jun 2020 18:04:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
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 Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Jun 2020 13:41:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/6266/ : Initial code review checks failed. See linked job for details on the failure. -- 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 Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Jun 2020 07:22:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 3: (1 comment) 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 dataSources_ = new CatalogObjectCache<>(); line too long (91 > 90) -- 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 Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Jun 2020 06:38:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Vihang Karajgaonkar has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. IMPALA-9824: MetastoreClientPool should be singleton This patch is a refactor-only patch. It changes the MetastoreClientPool into process wide singleton object. Currently, it is possible to instantiate multiple MetastoreClientPools although it doesn't seem to be a problem since all the places which instantiate it happen to run in separate processes. It would be better to enforce this in code. Making MetastoreClientPool a singleton also makes it easier to access from anywhere in the code without the need to explicitly pass its reference around which complicates the code flow unnecessarily. Additionally, this patch also introduces two new configurations coordinator_initial_hms_connections and catalog_initial_hms_connections which define the initial number of HMS clients which are created for the respective services. The default values of both the configurations are kept same as previous values from the code. Testing: 1. No new tests were added since this was mostly a refactor-only patch. 2. Ran existing core tests. Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f --- M be/src/catalog/catalog.cc M be/src/service/frontend.cc M be/src/util/backend-gflag-util.cc M common/thrift/BackendGflags.thrift M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java R fe/src/main/java/org/apache/impala/catalog/EmbeddedMetastoreClientPool.java M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java M fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java M fe/src/test/java/org/apache/impala/common/AbstractFrontendTest.java M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java A fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestTransientCatalog.java M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java 18 files changed, 229 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/16030/3 -- 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: newpatchset Gerrit-Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f Gerrit-Change-Number: 16030 Gerrit-PatchSet: 3 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 1: > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5971/ The test failures relate to the initialization order. It seems like Catalog takes in a MetastoreClientPool in the constructor unnecessarily. I see if I can update the patch to clean this up. -- 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: 1 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 09 Jun 2020 20:03:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 1: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5971/ -- 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: 1 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 09 Jun 2020 12:16:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5971/ DRY_RUN=true -- 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: 1 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 09 Jun 2020 07:01:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Quanlong Huang has posted comments on this change. (
http://gerrit.cloudera.org:8080/16030 )
Change subject: IMPALA-9824: MetastoreClientPool should be singleton
..
Patch Set 1: Code-Review+1
(4 comments)
LGTM. Just left some minor comments. I can bump to +2 after they are resolved.
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 com.google.common.annotations.VisibleForTesting;
nit: could you move this to line 36 with "com.google" package group?
http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@167
PS1, Line 167: if (MetastoreShim.getMajorVersion() > 2) {
: MetastoreShim.setHiveClientCapabilities();
: }
Is it intended to do this twice?
http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@178
PS1, Line 178: @return
nit: could you remove this empty return comment?
http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/16030/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@70
PS1, Line 70:
nit: redundant space
--
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: 1
Gerrit-Owner: Vihang Karajgaonkar
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Tue, 09 Jun 2020 02:19:58 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16030 ) Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6214/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 1 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 04 Jun 2020 18:14:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16030 Change subject: IMPALA-9824: MetastoreClientPool should be singleton .. IMPALA-9824: MetastoreClientPool should be singleton This patch is a refactor-only patch. It changes the MetastoreClientPool into process wide singleton object. Currently, it is possible to instantiate multiple MetastoreClientPool although it doesn't seem to be a problem since all the places which instantiate it happen to run in separate processes. It would be better to enforce this in code. Making MetastoreClientPool a singleton also makes it easier to access from anywhere in the code without the need to explicitly pass its reference around which complicates the code flow unnecessarily. Note that one side-effect of this patch is that number of initial clients that the pool instantiates is controlled using a configuration num_metadata_loading_threads which defaults to 16 instead of current default size of 10 in Catalog. Alternatively, we can introduce a separate config, but given that any metadata load operation needs a HMS client, we might be better off creating the clients same as num_metadata_loading_threads. Testing: 1. No new tests were added since this was mostly a refactor-only patch. 2. [WIP] Ran existing core tests. Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f --- M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java 8 files changed, 43 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/16030/1 -- 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: newchange Gerrit-Change-Id: I5477cba68b9053dfae8e7ed785f4d7519c12cd0f Gerrit-Change-Number: 16030 Gerrit-PatchSet: 1 Gerrit-Owner: Vihang Karajgaonkar
