Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17859 )
Change subject: IMPALA-10926: Improve catalogd consistency and self events detection ...................................................................... Patch Set 27: (6 comments) http://gerrit.cloudera.org:8080/#/c/17859/26/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/17859/26/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@328 PS26, Line 328: toryForSyncToLatestEvent(Catalog > remove if not needed. Ack http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@919 PS26, Line 919: } : org.apache.impala.catalog.Table tbl = null; : t > Not sure if I understand this condition correctly. Why are evaluating old s This is not the right condition because if sync to latest event id flag is set to true and the event is *not* a self event then the code from line no: 922 shall not get executed as tbl.setLastSyncedEventId(getEventId()) would get called while processing the event. To make this code more readable I can modify the if condition to: if (isSelfEvent && BackendConfig.INSTANCE.enableSyncToLatestEventOnDdls() ) { tbl = catalog_.getTable(getDbName(), getTableName()); if (tbl != null && catalog_.tryWriteLock(tbl)) { catalog_.getLock().writeLock().unlock(); if (tbl.getLastSyncedEventId() < getEventId()) { infoLog("is a self event. last synced event id for " + "table {} is {}. Setting it to {}", tbl.getFullName(), tbl.getLastSyncedEventId(), getEventId()); tbl.setLastSyncedEventId(getEventId()); } } Thoughts? http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@932 PS26, Line 932: > Why do we need to set the lastSyncedEventId here? Can we keep the scope of MetastoreEventFactory (and not EventFactoryForSyncToLatestEvent) skips processing an event which is self event. In that case, for a table, event factory should set the last synced event id to this self event id if enableSyncToLatestEventOnDdls() is set before skipping the processing of an event (more details in method comments) I am not sure what is the right place to do that and thats what this overridden method isSelfEvent does. http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java File fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java: http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/metastore/MetastoreServiceHandler.java@3088 PS26, Line 3088: String ms > remove if not needed. Ack http://gerrit.cloudera.org:8080/#/c/17859/26/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/17859/26/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@749 PS26, Line 749: if (existingTable != null) { : LOG.debug("EventId: {} Table {} was not added > Is this something that you are working on? I am not looking into it right now. But when I was working on it, the question in TODO comment crossed my mind. I am not sure if it is a valid scenario and for now I am thinking of adding a warning message if existing table's create event id does not match the event id passed in method argument. http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@929 PS26, Line 929: } > Is this TODO still unresolved? Please remove if it is. My understanding is Ack -- To view, visit http://gerrit.cloudera.org:8080/17859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36364e401911352c4666674eb98c8d61bbaae9b9 Gerrit-Change-Number: 17859 Gerrit-PatchSet: 27 Gerrit-Owner: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <kis...@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: Mon, 08 Nov 2021 12:40:54 +0000 Gerrit-HasComments: Yes
