Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12591 )
Change subject: IMPALA-7972 Detect self-events to avoid unnecessary invalidates ...................................................................... Patch Set 12: (9 comments) I think the new refactor makes sense. Just a few more nits and I think we are good to go. http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@743 PS12, Line 743: return result; throw DbNotFound...? http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@750 PS12, Line 750: result = tbl.getVersionsForInflightEvents(); : return result; merge? http://gerrit.cloudera.org:8080/#/c/12591/12/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/12591/12/fe/src/main/java/org/apache/impala/catalog/Table.java@655 PS12, Line 655: No-op if event processing is disabled doesn't look like it? http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@613 PS12, Line 613: pendingVersionNumbersFromCatalog_ = catalog_ One pending question. Since we are storing these version numbers in the table state, I think we are inherently relying on in-place table modifications (which is kinda true for most cases). However if someone issues an Invalidate, I think we replace the whole table object which essentially wipes of this state. I think the worst that could happen is probably some extra (and unnecessary) invalidates. Is my understanding right? Following the sequence of actions that * i think * could create this. t1. alter table. generates event e t2. invalidate table t3. load table. t4. event 'e' from HMS After t4, pendingVersionNumbersFromCatalog_ is an EMPTY_LIST and hence it kicks off another round of invalidation. Please correct me if I'm wrong. http://gerrit.cloudera.org:8080/#/c/12591/12/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/12591/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@521 PS12, Line 521: catalog_.getLock().writeLock().unlock(); this looks buggy, what is the corresponding lock for this? http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@527 PS12, Line 527: catalog_.getLock().writeLock().unlock() Move this too? http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@829 PS12, Line 829: catalog_.isExternalEventProcessingEnabled() Move this inside the addCatalogServiceidentifies()...? http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3253 PS12, Line 3253: setLastDdlTime update http://gerrit.cloudera.org:8080/#/c/12591/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3259 PS12, Line 3259: updateLastDdlTimeLocally nit: overrideDdlTime or something like that? -- To view, visit http://gerrit.cloudera.org:8080/12591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6db0d7f7fe465158fc8cb9d6b6b57a321827b353 Gerrit-Change-Number: 12591 Gerrit-PatchSet: 12 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Mar 2019 21:58:31 +0000 Gerrit-HasComments: Yes
