You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org> on 2020/06/04 17:26:39 UTC
[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 <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Jun 2020 20:03:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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<DataSource> 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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jun 2020 06:38:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Jun 2020 18:14:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Jun 2020 07:01:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jun 2020 13:41:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Jun 2020 02:19:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Jun 2020 23:58:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jun 2020 07:22:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Jun 2020 00:48:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jun 2020 19:53:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Jun 2020 12:16:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9824: MetastoreClientPool should be singleton
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <vi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jun 2020 18:04:39 +0000
Gerrit-HasComments: Yes