Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17697 )
Change subject: IMPALA-10801: Check the latest compaction Id before serving ACID table ...................................................................... Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@345 PS10, Line 345: hdfsTable_.writeLock().lock(); > This load fails after adding a precondition check for locking in HdfsTable. Ah, I see. The only code path which i see could hit this condition is when TableLoader is loading the table. TableLoader which loads the table in background does not need to take a table lock because in that case it loads the table in background and then atomically replaces it in the Db. The case where we need to take a lock on the table before reloading the table is when you are executing a ddl like alter. I don't think taking a lock here is the solution for this. I think in that case we should remove the Preconditions check since it is not applicable for all the code paths. http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/Table.java@592 PS10, Line 592: protected this probably is not needed if you remove the preconditions check. http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@115 PS10, Line 115: able.writeLock().lock(); > Same as icebergTable. After adding a precondition check for locking in Hdfs Please remove the added Preconditions check as per my comment earlier. http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java File fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java@526 PS10, Line 526: executeHiveSql can you add test coverage for major compaction too and partition level compactions too? http://gerrit.cloudera.org:8080/#/c/17697/10/testdata/cluster/ranger/setup/policy_5_revised.json File testdata/cluster/ranger/setup/policy_5_revised.json: http://gerrit.cloudera.org:8080/#/c/17697/10/testdata/cluster/ranger/setup/policy_5_revised.json@8 PS10, Line 8: 5 > Since this patch bumps up cdp version and the new version of ranger would c Yes, I think that would be better. Generally if there are associated changes with the BUILD number bump, we should commit them as a separate patch just for GBN bump so that we test them separately. -- To view, visit http://gerrit.cloudera.org:8080/17697 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I86a112a77980fef7f6238978bc9668a65262101e Gerrit-Change-Number: 17697 Gerrit-PatchSet: 10 Gerrit-Owner: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Comment-Date: Fri, 30 Jul 2021 18:46:27 +0000 Gerrit-HasComments: Yes
