You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Yida Wu (Code Review)" <ge...@cloudera.org> on 2025/05/21 05:06:10 UTC

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Yida Wu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22930

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................

WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Replace global metadata DDL lock with CatalogActionLocker for
fine grained locking of DDL operations. The previous global lock
could block the entire catalog server during slow HMS operations.

We still keep the existing table and database locks. However,
this new locker focuses on the whole operation, it grabs the
needed locks at the beginning and holds them until the operation
finishes. The new locker uses database and table level locks,
allowing DDL operations to run at the same time when possible,
using shared and exclusive locks. This helps reduce blocking and
improves performance.

This locker grabs locks early, which may hold them longer than
late locking but should be good enough for ddls like CREATE_DB or
DROP_TABLE with database and table level locks, especially
compared to the current global locking. It could simplify
correctness and the centralized structure makes it easier to see
which operation holds which locks, improving debugging. In the
future, it may also help reject duplicate actions early without
long waiting (not included in this patch).

Right now, only the operations that used the global metadata DDL
lock are updated to use CatalogActionLocker. We may use it for more
operations later.

Tests:
Adds CatalogActionLockerTest for concurrency and correctness check.
Passed custom_cluster/test_concurrent_ddls.py.
TODO more tests.

Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
---
A fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/metastore/CatalogMetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
A fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java
6 files changed, 904 insertions(+), 71 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/22930/1
--
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/19260/ : 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/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 May 2025 05:36:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 4:

(2 comments)

Thanks Michael for the review. I think the basic idea here is to use dynamic locking per operation, instead of using static sharded locks

http://gerrit.cloudera.org:8080/#/c/22930/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/22930/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1898
PS3, Line 1898:         throws CatalogException, MetastoreNotificationNeedsInvalidateException {
> DatabaseNotFoundException is a child of CatalogException, so could be remov
Done


http://gerrit.cloudera.org:8080/#/c/22930/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2258
PS3, Line 2258:       }
> The exception really has no additional useful context?
Added a log.



-- 
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 22 May 2025 22:46:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................

WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Replace global metadata DDL lock with CatalogActionLocker for
fine grained locking of DDL operations. The previous global lock
could block the entire catalog server during slow HMS operations.

Right now, only the operations that used the global metadata DDL
lock are updated to use CatalogActionLocker. We may use it for more
operations later.

Tests:
Adds CatalogActionLockerTest for concurrency and correctness check.
Passed custom_cluster/test_concurrent_ddls.py.
TODO more tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/22930/2
--
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................

WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Replace global metadata DDL lock with CatalogActionLocker for
fine grained locking of DDL operations. The previous global lock
could block the entire catalog server during slow HMS operations.

Right now, only the operations that used the global DDL lock are
updated to use CatalogActionLocker. We may use it for more
operations later.

Tests:
Adds CatalogActionLockerTest for concurrency and correctness check.
Passed custom_cluster/test_concurrent_ddls.py.
TODO more tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/22930/4
--
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 1:

(34 comments)

gerrit-auto-critic failed. You can reproduce it locally using command:

  python3 bin/jenkins/critique-gerrit-review.py --dryrun

To run it, you might need a virtual env with Python3's venv installed.

http://gerrit.cloudera.org:8080/#/c/22930/1/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/22930/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5399
PS1, Line 5399:    * TODO: update the comments 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java:

http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@37
PS1, Line 37:   private static final Logger LOG = LoggerFactory.getLogger(CatalogActionLockerTest.class);
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@62
PS1, Line 62:       TestTask(String name, CatalogActionLocker.OperationType opType, String db, String table, long sleepBefore, long sleepAfter) {
line too long (131 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@75
PS1, Line 75:           LOG.info("Thread {}: Starting, op={}, db={}, table={}", name, opType, db, table != null ? table : "null");
line too long (116 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@85
PS1, Line 85:             LOG.error("Thread {}: Exception for op={}: {}", name, opType, e.getMessage(), e);
line too long (93 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@96
PS1, Line 96:                 LOG.error("Thread {}: Exception releasing lock for op={}: {}", name, opType, e.getMessage(), e);
line too long (112 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@101
PS1, Line 101:             LOG.info("Thread {}: No lock acquired for op={}, skipping release", name, opType);
line too long (94 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@104
PS1, Line 104:           LOG.info("Thread {}: Counted down latch for op={}, remaining={}", name, opType, latch.getCount());
line too long (108 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@110
PS1, Line 110:     executor.submit(new TestTask("T1", CatalogActionLocker.OperationType.CREATE_TABLE, "mydb", "tbl1", 0, 2000));
line too long (113 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@113
PS1, Line 113:     executor.submit(new TestTask("T2", CatalogActionLocker.OperationType.CREATE_TABLE, "mydb", "tbl1", 500, 1000));
line too long (115 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@116
PS1, Line 116:     executor.submit(new TestTask("T3", CatalogActionLocker.OperationType.REFRESH_FUNCTIONS, "mydb", null, 600, 1000));
line too long (118 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@119
PS1, Line 119:     executor.submit(new TestTask("T4", CatalogActionLocker.OperationType.CREATE_TABLE_BY_EVENT, "mydb", "tbl1", 700, 500));
line too long (123 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@122
PS1, Line 122:     executor.submit(new TestTask("T5", CatalogActionLocker.OperationType.DROP_DB, "mydb", null, 800, 500));
line too long (107 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@125
PS1, Line 125:     executor.submit(new TestTask("T6", CatalogActionLocker.OperationType.CREATE_DB, "mydb2", null, 900, 500));
line too long (110 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@128
PS1, Line 128:     executor.submit(new TestTask("T7", CatalogActionLocker.OperationType.DROP_TABLE, "mydb", "tbl2", 1000, 500));
line too long (113 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@131
PS1, Line 131:     executor.submit(new TestTask("T8", CatalogActionLocker.OperationType.RENAME_TABLE_BY_EVENT, "mydb", "tbl2", 1100, 500));
line too long (124 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@134
PS1, Line 134:     executor.submit(new TestTask("T9", CatalogActionLocker.OperationType.DROP_TABLE_BY_EVENT, "mydb", "tbl2", 1200, 500));
line too long (122 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@137
PS1, Line 137:     executor.submit(new TestTask("T10", CatalogActionLocker.OperationType.CREATE_DB_BY_EVENT, "mydb3", null, 1300, 500));
line too long (121 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@141
PS1, Line 141:     LOG.info("Latch await finished, allCompleted={}, remaining count={}", allCompleted, latch.getCount());
line too long (106 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@169
PS1, Line 169:         locker.assertAction(CatalogActionLocker.OperationType.CREATE_TABLE, "mydb", "tbl1");
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@187
PS1, Line 187:           locker.releaseAction(CatalogActionLocker.OperationType.CREATE_TABLE, "mydb", "tbl1");
line too long (95 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@206
PS1, Line 206:       locker.assertAction(CatalogActionLocker.OperationType.RENAME_TABLE_BY_EVENT, "mydb", "tbl1", "newdb", "tbl2");
line too long (116 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@207
PS1, Line 207:       LOG.info("Acquired RENAME_TABLE_BY_EVENT (S on mydb, X on mydb.tbl1, S on newdb, X on newdb.tbl2)");
line too long (106 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@210
PS1, Line 210:       locker.assertAction(CatalogActionLocker.OperationType.DROP_TABLE_BY_EVENT, "mydb", "tbl1");
line too long (97 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@214
PS1, Line 214:       locker.releaseAction(CatalogActionLocker.OperationType.DROP_TABLE_BY_EVENT, "mydb", "tbl1");
line too long (98 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@218
PS1, Line 218:       locker.assertAction(CatalogActionLocker.OperationType.CREATE_TABLE_BY_EVENT, "newdb", "tbl2");
line too long (100 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@222
PS1, Line 222:       locker.releaseAction(CatalogActionLocker.OperationType.CREATE_TABLE_BY_EVENT, "newdb", "tbl2");
line too long (101 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@226
PS1, Line 226:       locker.releaseAction(CatalogActionLocker.OperationType.RENAME_TABLE_BY_EVENT, "mydb", "tbl1", "newdb", "tbl2");
line too long (117 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@232
PS1, Line 232:       LOG.error("Unexpected exception in testRenameTableByEventReentrancy: {}", e.getMessage(), e);
line too long (99 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@246
PS1, Line 246:           locker.assertAction(CatalogActionLocker.OperationType.CREATE_TABLE_BY_EVENT, "newdb", "tbl2");
line too long (104 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@247
PS1, Line 247:           LOG.info("Thread T1: Acquired CREATE_TABLE_BY_EVENT (S on newdb, X on newdb.tbl2)");
line too long (94 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@258
PS1, Line 258:           locker.assertAction(CatalogActionLocker.OperationType.RENAME_TABLE_BY_EVENT, "mydb", "tbl1", "newdb", "tbl2");
line too long (120 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@277
PS1, Line 277:         locker.releaseAction(CatalogActionLocker.OperationType.CREATE_TABLE_BY_EVENT, "newdb", "tbl2");
line too long (103 > 90)


http://gerrit.cloudera.org:8080/#/c/22930/1/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@287
PS1, Line 287:       LOG.error("Unexpected exception in testRenameTableByEventRollback: {}", e.getMessage(), e);
line too long (97 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 May 2025 05:07:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 2:

(1 comment)

gerrit-auto-critic failed. You can reproduce it locally using command:

  python3 bin/jenkins/critique-gerrit-review.py --dryrun

To run it, you might need a virtual env with Python3's venv installed.

http://gerrit.cloudera.org:8080/#/c/22930/2/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java:

http://gerrit.cloudera.org:8080/#/c/22930/2/fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java@230
PS2, Line 230:           "Acquired RENAME_TABLE_BY_EVENT (S on mydb, X on mydb.tbl1, S on newdb, X on newdb.tbl2)");
line too long (101 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 May 2025 05:14:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/19295/ : 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/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 May 2025 22:11:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 3:

(2 comments)

I'm interested, but only time for a cursory look so far.

http://gerrit.cloudera.org:8080/#/c/22930/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/22930/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1898
PS3, Line 1898:         throws DatabaseNotFoundException, MetastoreNotificationNeedsInvalidateException,
DatabaseNotFoundException is a child of CatalogException, so could be removed.


http://gerrit.cloudera.org:8080/#/c/22930/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2258
PS3, Line 2258:         // do nothing, dbAdded failed
The exception really has no additional useful context?



-- 
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 May 2025 16:49:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/22930/5/fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java
File fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java:

http://gerrit.cloudera.org:8080/#/c/22930/5/fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java@125
PS5, Line 125:     final Map<ResourceKey, Action> dbActions = new HashMap<>();
             :     final Map<ResourceKey, Action> tableActions = new HashMap<>();
We need to make sure no leaks in these maps, i.e. if a db/table no longer exists in catalog, the corresponding key-value pair should also be removed.

Haven't looked into the details but I have two questions:
- When a db is dropped, will all the key-value pairs of its underlying tables be dropped as well?
- When creating a db/table fails in HMS side, will the corresponding key-value pair be removed?


http://gerrit.cloudera.org:8080/#/c/22930/5/fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java@147
PS5, Line 147: NUM_PARTITIONS
We need this to be configurable.


http://gerrit.cloudera.org:8080/#/c/22930/5/fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java@261
PS5, Line 261:       try {
We'd better add a thread annotation (ThreadNameAnnotator) for this so jstacks can show what the thread is waiting for.


http://gerrit.cloudera.org:8080/#/c/22930/5/fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java@273
PS5, Line 273:       action = actionMap.get(key);
Why do we need to get it again here? Will other threads replace it?


http://gerrit.cloudera.org:8080/#/c/22930/5/fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java@392
PS5, Line 392: e instanceof CatalogException
e is always CatalogException (declared on L384).



-- 
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Jun 2025 13:52:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/19387/ : 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/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Jun 2025 00:14:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/22930 )

Change subject: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................

IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

The patch introduces CatalogActionLocker to replace global metadata
DDL lock for fine grained locking of DDL operations. The previous
global lock could block the entire catalog server during slow HMS
operations.

Today, HMS is expected to handle concurrent access safely, making
the global metadata lock largely unnecessary for that purpose.
However, this global lock also guarded local catalog structures,
for example, checking local database and table before create/drop
under the lock to ensure consistent behavior. Without coordination,
this could lead to misleading errors. Using CatalogActionLocker
can maintain consistency while allowing more concurrency.

We still keep the existing table and database locks. However,
this new locker focuses on the whole operation, it grabs the
needed locks at the beginning and holds them until the operation
finishes. The new locker uses database and table level locks,
allowing DDL operations to run at the same time when possible,
using shared and exclusive locks. This helps reduce blocking and
improves performance. In the future, it may also help reject
duplicate actions early without long waiting.

Right now, only the operations that used the global DDL lock are
updated to use CatalogActionLocker, including drop/create table,
drop/create database, rename table from events and so on. We may
use it for more operations later.

CatalogMetastoreServiceHandler and MetastoreServiceHandler still
use the global metadata lock. But since they are not currently
used by upstream Impala and don't have much impact, we can update
them later.

Tests:
Passed exhaustive tests.
Adds CatalogActionLockerTest for concurrency and correctness check.
TODO more tests.

Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
---
A fe/src/main/java/org/apache/impala/catalog/CatalogActionLocker.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
A fe/src/test/java/org/apache/impala/catalog/CatalogActionLockerTest.java
M tests/query_test/test_observability.py
5 files changed, 1,123 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/22930/5
-- 
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/19261/ : 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/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 May 2025 05:39:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/19262/ : 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/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 May 2025 05:44:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/22930 )

Change subject: WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking
......................................................................

WIP: IMPALA-6406: Add CatalogActionLocker for fine grained DDL locking

Replace global metadata DDL lock with CatalogActionLocker for
fine grained locking of DDL operations. The previous global lock
could block the entire catalog server during slow HMS operations.

Right now, only the operations that used the global metadata DDL
lock are updated to use CatalogActionLocker. We may use it for more
operations later.

Tests:
Adds CatalogActionLockerTest for concurrency and correctness check.
Passed custom_cluster/test_concurrent_ddls.py.
TODO more tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/22930/3
--
To view, visit http://gerrit.cloudera.org:8080/22930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ed48d92333787f75b64b8bd878834612433f6f8
Gerrit-Change-Number: 22930
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>