Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15818 )
Change subject: IMPALA-9512: Full ACID Milestone 2: Validate rows against the valid write id list ...................................................................... Patch Set 15: (6 comments) Thanks for the comments, Vihang! http://gerrit.cloudera.org:8080/#/c/15818/13/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/15818/13/fe/src/main/java/org/apache/impala/catalog/Table.java@580 PS13, Line 580: if (getMetaStoreTable() != null && > Is it intentional that we always send the ValidWriteIdList irrespective of Honestly I didn't put too much effort into thinking about it. My only thought was that it should be fairly cheap to send, so probably it'd be best to send it allways. We can change it in the future if it becomes an issue. http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/util/AcidUtils.java@387 PS13, Line 387: if (parsedDelta.minWriteId <= baseWriteId) { : Preconditions.checkState(parsedDelta.maxWriteId <= baseWriteId); : it.remove(); : continue; : } > we could use some comments here. Is the code removing all the delta directo It can happen after major compaction, INSERT OVERWRITE, and TRUNCATE because they are creating new base directories that supercede everything before them. Btw we did this already, I just moved this code from Base L320. http://gerrit.cloudera.org:8080/#/c/15818/13/fe/src/main/java/org/apache/impala/util/AcidUtils.java@398 PS13, Line 398: MetaException > Change to CatalogException? Yeah, probably it would be a better fit. Will do that in the following milestones. http://gerrit.cloudera.org:8080/#/c/15818/15/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/15818/15/fe/src/main/java/org/apache/impala/util/AcidUtils.java@191 PS15, Line 191: if (!isTxnValid(pd.visibilityTxnId)) return false; > I am curious to understand this. The writeIdList that we load is pertaining INSERT-only tables can only be major compacted, so it was enough to check the visibility txn id of the base directories only. This milestone adds read support for minor compacted delta directories (which is full ACID-only) which also have a visibility txn id for the same reason, i.e. compactions don't allocate write ids. We only need to use validTxnIdList for checking the validitiy of compactions. http://gerrit.cloudera.org:8080/#/c/15818/15/fe/src/main/java/org/apache/impala/util/AcidUtils.java@246 PS15, Line 246: visibilityTxnId > Just for sake my understanding, is this needed to support minor compactions Yes. http://gerrit.cloudera.org:8080/#/c/15818/15/fe/src/main/java/org/apache/impala/util/AcidUtils.java@380 PS15, Line 380: baseWriteId > Is it possible that baseWriteId is equal to SENTINEL_BASE_WRITE_ID here? It's possible if we don't have a base dir, only deltas and/or original files. -- To view, visit http://gerrit.cloudera.org:8080/15818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5ed74585a2d73ebbcee763b0545be4412926299d Gerrit-Change-Number: 15818 Gerrit-PatchSet: 15 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 22 May 2020 13:45:01 +0000 Gerrit-HasComments: Yes
