You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2024/03/07 11:49:45 UTC
[Impala-ASF-CR] WIP IMPALA-12831: Add lock for partition map in HdfsTable
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21115
Change subject: WIP IMPALA-12831: Add lock for partition map in HdfsTable
......................................................................
WIP IMPALA-12831: Add lock for partition map in HdfsTable
HdfsTable.toMinimalTCatalogObject() is not always invoked with holding
the table lock, e.g. in invalidating a table, we could replace an
HdfsTable instance with an IncompleteTable instance. We then invoke
HdfsTable.toMinimalTCatalogObject() to get the removed catalog object.
However, the HdfsTable instance could be modified in the meantime by a
concurrent DDL/DML that would reload it, e.g. a REFRESH statement. This
causes HdfsTable.toMinimalTCatalogObject() failed by
ConcurrentModificationException on the column/partition list.
There are two parts in HdfsTable.toMinimalTCatalogObject() that could be
failed by concurrent modifications. One is adding the columns, the other
is adding the partitions. The columns are not used in the result
consumers so we can simply ignore them. To protect reading the partition
map, this patch adds a ReentrantReadWriteLock in HdfsTable for it.
Refactor some code paths to avoid holding the lock during external RPCs,
e.g. loading file metadata.
TODO:
HdfsTable.getPartitions() returns the map values externally. To protect
its usage without exposing the partitionMapLock outside HdfsTable, we
have to make sure it's only used in impalad or tests.
Currently, there are several code paths in catalogd that use it, e.g.
FeFsTable.Utils.getPartitionFromThriftPartitionSpec() which is used in
many catalogd code paths. As a workaround, a deep copy of the value list
are returned in getPartitions().
Tests
- Added e2e test
- TODO: run CORE tests
Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
M tests/custom_cluster/test_concurrent_ddls.py
7 files changed, 358 insertions(+), 173 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/21115/1
--
To view, visit http://gerrit.cloudera.org:8080/21115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
Gerrit-Change-Number: 21115
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] WIP IMPALA-12831: Add lock for partition map in HdfsTable
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21115
to look at the new patch set (#2).
Change subject: WIP IMPALA-12831: Add lock for partition map in HdfsTable
......................................................................
WIP IMPALA-12831: Add lock for partition map in HdfsTable
HdfsTable.toMinimalTCatalogObject() is not always invoked with holding
the table lock, e.g. in invalidating a table, we could replace an
HdfsTable instance with an IncompleteTable instance. We then invoke
HdfsTable.toMinimalTCatalogObject() to get the removed catalog object.
However, the HdfsTable instance could be modified in the meantime by a
concurrent DDL/DML that would reload it, e.g. a REFRESH statement. This
causes HdfsTable.toMinimalTCatalogObject() failed by
ConcurrentModificationException on the column/partition list.
There are two parts in HdfsTable.toMinimalTCatalogObject() that could be
failed by concurrent modifications. One is adding the columns, the other
is adding the partitions. The columns are not used in the result
consumers so we can simply ignore them. To protect reading the partition
map, this patch adds a ReentrantReadWriteLock in HdfsTable for it.
Refactor some code paths to avoid holding the lock during external RPCs,
e.g. loading file metadata.
TODO:
HdfsTable.getPartitions() returns the map values externally. To protect
its usage without exposing the partitionMapLock outside HdfsTable, we
have to make sure it's only used in impalad or tests.
Currently, there are several code paths in catalogd that use it, e.g.
FeFsTable.Utils.getPartitionFromThriftPartitionSpec() which is used in
many catalogd code paths. As a workaround, a deep copy of the value list
are returned in getPartitions().
Tests
- Added e2e test
- TODO: run CORE tests
Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
M tests/custom_cluster/test_concurrent_ddls.py
7 files changed, 360 insertions(+), 173 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/21115/2
--
To view, visit http://gerrit.cloudera.org:8080/21115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
Gerrit-Change-Number: 21115
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-12831: Add lock for partition map in HdfsTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21115 )
Change subject: WIP IMPALA-12831: Add lock for partition map in HdfsTable
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/21115/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/21115/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1161
PS1, Line 1161: private void updatePartitionNoLock(HdfsPartition.Builder partBuilder) throws CatalogException {
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/21115/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1271
PS1, Line 1271: HdfsPartition hdfsPartition = dropPartitionNoLock(partition, removeCacheDirective);
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/21115/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1655
PS1, Line 1655: // Remove partitions that don't exist in the Hive Metastore. These are partitions
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/21115/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1663
PS1, Line 1663: // add them to the list of updatedPartitions so that they are reloaded later.
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/21115/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2551
PS1, Line 2551: tHdfsPartition.isSetFile_desc() ? tHdfsPartition.getFile_desc().size() : 0;
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/21115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
Gerrit-Change-Number: 21115
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2024 11:50:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-12831: Add lock for partition map in HdfsTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21115 )
Change subject: WIP IMPALA-12831: Add lock for partition map in HdfsTable
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/15435/ : 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/21115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
Gerrit-Change-Number: 21115
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2024 12:18:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-12831: Add lock for partition map in HdfsTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21115 )
Change subject: WIP IMPALA-12831: Add lock for partition map in HdfsTable
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21115/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/21115/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1665
PS2, Line 1665: // add them to the list of updatedPartitions so that they are reloaded later.
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/21115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
Gerrit-Change-Number: 21115
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2024 12:17:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-12831: Add lock for partition map in HdfsTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21115 )
Change subject: WIP IMPALA-12831: Add lock for partition map in HdfsTable
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/15436/ : 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/21115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7466c291e4619b1ceac325b84f1eadc598aef29e
Gerrit-Change-Number: 21115
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2024 12:45:09 +0000
Gerrit-HasComments: No