Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22789 )
Change subject: IMPALA-13631: alterTableOrViewRename shouldn't hold catalog versionLock during external RPCs ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/22789/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/22789/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1216 PS1, Line 1216: catalog_.getLock().writeLock().unlock(); > The relationship between this and tryWriteLock is confusing, but I think I This pattern is commented at https://github.com/apache/impala/blob/eb649628a198704db05db637d145af0ce2bb6587/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L295-L310 But I understand the confusion. tryWriteLock(tbl, ...) sounds like it just acquires the table lock. But it actually acquires the catalogVersion lock that we have to release it here. Maybe we should improve the method name to reveal this, e.g. tryLockCatalogAndTable(). We can rename it in a refactoring patch to keep this patch easy for backporting. http://gerrit.cloudera.org:8080/#/c/22789/1/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: http://gerrit.cloudera.org:8080/#/c/22789/1/tests/metadata/test_ddl.py@486 PS1, Line 486: @UniqueDatabase.parametrize(num_dbs=2) > I'm not clear how this is adding to the test. This is defined in tests/common/parametrize.py. We use num_dbs=2 here to create two unique databases: https://github.com/apache/impala/blob/eb649628a198704db05db637d145af0ce2bb6587/tests/conftest.py#L351-L354 http://gerrit.cloudera.org:8080/#/c/22789/1/tests/metadata/test_ddl.py@509 PS1, Line 509: new_tbl_name = "{}2.tbl_{}".format(unique_database, i) > Do you need to do anything to make sure this exists? Or does the DB get cre The second db is also created by the unique_database fixture: https://github.com/apache/impala/blob/eb649628a198704db05db637d145af0ce2bb6587/tests/conftest.py#L382-L384 https://github.com/apache/impala/blob/eb649628a198704db05db637d145af0ce2bb6587/tests/conftest.py#L415-L417 -- To view, visit http://gerrit.cloudera.org:8080/22789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie5f443b1e167d96024b717ce70ca542d7930cb4b Gerrit-Change-Number: 22789 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Mon, 21 Apr 2025 12:30:32 +0000 Gerrit-HasComments: Yes
