Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17081 )
Change subject: IMPALA-10512: ALTER TABLE ADD PARTITION should bump the write id for ACID tables ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/17081/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17081/1//COMMIT_MSG@9 PS1, Line 9: should bump the write id > Don't we also need a lock in this case? For example currently this can happ TRUNCATE doesn't drop partitions. http://gerrit.cloudera.org:8080/#/c/17081/1//COMMIT_MSG@9 PS1, Line 9: ADD PARTITION > Don't we need to do the same in DROP PARTITION? DROP PARTITION is not supported for ACID tables. http://gerrit.cloudera.org:8080/#/c/17081/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/17081/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3187 PS1, Line 3187: long txnId = MetastoreShim.openTransaction(msClient); > are we opening a transaction post adding the partitions? Shouldn't the flow Table metadata operations are not handled in a transactional way AFAIK, i.e. they are not associated with ACID transactions. So even if we've opened a transaction before adding the partition, after the partition is added, abort transaction would not revert the operation. So doing it in an open transaction, allocate write id, add partition, commit txn sequence would make the false impression that this operation is transactional. I'm not sure if there's any benefit doing it in that sequence. Doing it separately saves us from opening a transaction in case of failures, and it also makes it clear that add partition is not part of an ACID transaction. http://gerrit.cloudera.org:8080/#/c/17081/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3188 PS1, Line 3188: allocateTableWriteId > What happens if allocateTableWriteId throws a TransactionException? In that case the new partition will be added, but we don't increase the write id for the table. It might cause minor glitches for replicated tables, i.e. SHOW PARTITIONS might not display the newly added empty partition for a replicated table. I'm not sure if we want to add an UNDO operation to remove partitions in this case. -- To view, visit http://gerrit.cloudera.org:8080/17081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iad247008b7c206db00516326c1447bd00a9b34bd Gerrit-Change-Number: 17081 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 19 Feb 2021 17:25:15 +0000 Gerrit-HasComments: Yes
