You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sourabh Goyal (Code Review)" <ge...@cloudera.org> on 2021/07/20 16:26:13 UTC
[Impala-ASF-CR] [WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
Sourabh Goyal has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17703
Change subject: [WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
......................................................................
[WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 333 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17703/1
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 1
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
[Impala-ASF-CR] [WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17703/1/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/1/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@250
PS1, Line 250: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(),
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/17703/1/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@357
PS1, Line 357: Partition partition) throws InvalidOperationException, MetaException, TException {
line too long (113 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 1
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 16:26:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@247
PS3, Line 247: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(),
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 10:55:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9135/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 17:16:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17703
to look at the new patch set (#2).
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
[WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 333 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17703/2
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 2
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 3:
Requesting an early review on the approach.
Sharing few details on the approach taken:
All HMS APIs for which we need to sync to the latest envent id are overriden in CatalogMetastoreServiceHandler. If either enableCatalogdHMSCache or syncToLatestEventId flag is false, we fallback to pass through api in MetastoreServiceHandler.
One api to look into is exchange_partition for which we need to take lock on both source and destination table atomically. Since there was no api in CatalogServiceCatalog for acquiring lock on multiple tables, I defined one in this patch.
Please review it and share your feedback. Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 11:16:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, kishen@cloudera.com, Yu-Wen Lai, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17703
to look at the new patch set (#4).
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
[WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 371 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17703/4
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
[Impala-ASF-CR] [WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acqurie table/database lock in metastore server before any hms operation
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9125/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 1
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 16:44:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9226/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 6
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Aug 2021 10:05:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
kishen@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@444
PS3, Line 444: tableInfo.toString());
Can you wrap the lines from 435 to 444 within an if isDebugEnabled ?
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@457
PS3, Line 457: // except last
Why you are you releasing the write locks here ?
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@186
PS3, Line 186: // long newCatalogVersion = catalog_.incrementAndGetCatalogVersion();
Please remove the line, if you are not planning to use newCatalogVersion?
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@192
PS3, Line 192: catalogOpExecutor_.syncToLatestEventId(Db db, apiName);
Can you add one sample test case, where CatalogOpExecutor and MetastoreServiceHanlder interact with the same table at the same time ?
I just want to see how it would look like.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@219
PS3, Line 219: catalog_.getLock().writeLock().unlock();
Do we still need Catalog level lock, since we now have global table level lock ?
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@247
PS3, Line 247: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(),
> line too long (92 > 90)
Please take care of these.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@396
PS3, Line 396: catalogOpExecutor_.UnlockWriteLockIfErronouslyLocked();
UnlockWriteLockIfErronouslyLocked what does this mean ?
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@411
PS3, Line 411: CatalogException e =
Can you not throw TException ?
How CatalogException is handled up in the chain ?
http://gerrit.cloudera.org:8080/#/c/17703/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/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1401
PS3, Line 1401: protected void throwException(Exception cause, String apiName)
May be rethrowException ?
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2871
PS3, Line 2871: private long getCurrentEventId(MetaStoreClient msClient) throws TException {
Where are you using this ?
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 16:54:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has abandoned this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Abandoned
Consolidating multiple patches into one i.e https://gerrit.cloudera.org/c/17859/
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 9
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9134/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 11:12:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@444
PS3, Line 444: tableInfo.toString());
> Can you wrap the lines from 435 to 444 within an if isDebugEnabled ?
Sure !
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@457
PS3, Line 457: // except last
> Why you are you releasing the write locks here ?
So that the caller of this method has to call unlock() only once.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@186
PS3, Line 186: }
> Please remove the line, if you are not planning to use newCatalogVersion?
Ack
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@192
PS3, Line 192: LOG.debug("Successfully executed HMS API: " + apiName);
> Do we need to sync table/database to latest event in this class? If we don'
@Kishen: Sure, will add a UT. For now it would be a no-op since we haven't implemented syncToLatestEventId() yet. I was planning to add tests in implementation patch of syncToLatestEventId().
@Yu-Wen: Why should we defer the update? If we do so, then it is possible that we may have to sync up lots of HMS events in the next read leading to delays in the read. However if we sync up with every HMS operation then we may have to apply only small number of updates.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@219
PS3, Line 219: // get released in finally block
> Do we still need Catalog level lock, since we now have global table level l
Rename requires removing an old table and adding back a new table in cache, that's why global write lock is required to make sure that cache remains unchanged during this operation.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@247
PS3, Line 247: String apiName = HmsApiNameEnum.ADD_PARTITION.apiName();
> Please take care of these.
Ack
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@396
PS3, Line 396: } finally {
> UnlockWriteLockIfErronouslyLocked what does this mean ?
It forcefully unlocks the version.writeLock() if the lock was held by current thread by this time. It is just for precaution to make sure that version lock is always released in the end since it is a global lock.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@411
PS3, Line 411: // should it be an internal exception?
> Can you not throw TException ?
at line no. 414 - throwException() throws MetaException - a type of TException.
http://gerrit.cloudera.org:8080/#/c/17703/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/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@1401
PS3, Line 1401: protected void throwException(Exception cause, String apiName)
> May be rethrowException ?
Sure, will rename it.
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@2871
PS3, Line 2871: private long getCurrentEventId(MetaStoreClient msClient) throws TException {
> Where are you using this ?
For now, I am not using it anywhere in the patch and the implementation already existed. I have just moved it up.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Jul 2021 10:45:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Yu-Wen Lai (Code Review)" <ge...@cloudera.org>.
Yu-Wen Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@192
PS3, Line 192: LOG.debug("Successfully executed HMS API: " + apiName);
> @Kishen: Sure, will add a UT. For now it would be a no-op since we haven't
@Sourabh: I was thinking some methods might not need to wait until events are synced up but let eventProcessor to do that in the background. Given that there should be only a small number of updates, I agree this way is better and cleaner to keep each function has similar logic.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Jul 2021 20:38:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17703/6/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/6/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@247
PS6, Line 247: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(),
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 6
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Aug 2021 09:52:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 8:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@428
PS8, Line 428: /*
: Acquire write lock on multiple tables
: If the lock couldn't be acquired on any table,
: then release the lock on previous tables and
: return false. Otherwise return true if writelocks
: were acquried on all tables
: */
nit, Can you please switch to using java doc style comments like other methods? The advantage of using that is that IDE can render the comment when inspecting a method.
/**
* comment ..
*/
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@449
PS8, Line 449: for(int i = 0; i < numTables; i++) {
: Table tbl = tables[i];
: if (!tryWriteLock(tbl)) {
: LOG.debug("Could not acquire write lock on table: " + tbl.getFullName());
: // unlock previously locked tables
: for(int j = 0; j < i; j++) {
: tables[j].releaseWriteLock();
: }
: return false;
: }
: // unlock version write lock for all tables
: // except last
: if (i < numTables-1) {
: versionLock_.writeLock().unlock();
: }
: }
I think you should make sure that this method releases the versionLock before coming out even in case of error conditions (line 457). See usage of method UnlockWriteLockIfErronouslyLocked() for example.
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@46
PS8, Line 46: g.*
nit, please avoid wildcard imports.
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@190
PS8, Line 190: OG.debug("Successfully executed HMS API: " + apiName);
nit, please use parameterized logging whenever possible. (see http://www.slf4j.org/faq.html#logging_performance for reference). Additionally, the log should have the database name to make more sense of it.
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@226
PS8, Line 226: LOG.debug("Successfully executed HMS api:" + apiName)
use parameterized logging format, add table name in the log statement.
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@251
PS8, Line 251: LOG
use parameterized logging format, add table name and partition name in the log statement.
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@352
PS8, Line 352: if (!catalog_.tryWriteLock(new Table[] {srcTbl, destinationTbl})) {
: rethrowException(new CatalogException("Couldn't acquire lock on tables: "
: + srcTbl.getFullName() + ", " + destinationTbl.getFullName()), apiName);
: }
:
: Partition exchangedPartition = super.exchange_partition(partitionSpecMap, sourcedb,
: sourceTbl, destDb, destTbl);
move these into try block so that locks are released in the finally block?
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@590
PS8, Line 590: org.apache.impala.catalog.Table tbl;
add java doc for this method. Please be specific about the behaviour of catalog version not being released and that it is callers responsibility to make sure that version lock is released.
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17703/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6236
PS8, Line 6236: Preconditions.checkState(tbl.isWriteLockedByCurrentThread());
: Preconditions.checkState(!(tbl instanceof IncompleteTable) &&
: tbl.isLoaded());
are we handling these exceptions on the caller side?
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 8
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Aug 2021 18:59:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Yu-Wen Lai (Code Review)" <ge...@cloudera.org>.
Yu-Wen Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/3/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@192
PS3, Line 192: LOG.debug("Successfully executed HMS API: " + apiName);
> Can you add one sample test case, where CatalogOpExecutor and MetastoreServ
Do we need to sync table/database to latest event in this class? If we don't directly update cache here, is it possible to delay the sync up operation until next read?
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 18:59:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17703
to look at the new patch set (#3).
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
[WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 370 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17703/3
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
File fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java:
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java@248
PS4, Line 248: org.apache.impala.catalog.Table tbl = getTableAndAcquireWriteLock(partition.getDbName(),
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 16:57:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@428
PS4, Line 428: Acquire write lock on multiple tables
> It might be good idea to move lock related methods to a separate class. Thi
Sure. We can work on refactoring in a separate patch.
Thoughts?
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6201
PS4, Line 6201:
> Are you going to flesh out these methods in this PR or that will be a separ
I am thinking of creating a separate PR which would implement and use these methods. Want to restrict this PR to only acquiring db/table locks.
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Jul 2021 14:36:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
kishen@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17703 )
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@428
PS4, Line 428: Acquire write lock on multiple tables
It might be good idea to move lock related methods to a separate class. This class is becoming too large, which affects the readability.
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17703/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6201
PS4, Line 6201:
Are you going to flesh out these methods in this PR or that will be a separate PR ?
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Jul 2021 08:06:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, kishen@cloudera.com, Yu-Wen Lai, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17703
to look at the new patch set (#6).
Change subject: [WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
......................................................................
[WIP]: Initial commit to acquire table/database lock in metastore server before any HMS operation
Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/catalog/metastore/HmsApiNameEnum.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 574 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17703/6
--
To view, visit http://gerrit.cloudera.org:8080/17703
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I085eab20db61282daf4549ddbcc018aaf63cc361
Gerrit-Change-Number: 17703
Gerrit-PatchSet: 6
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Yu-Wen Lai <yu...@cloudera.com>