Vihang Karajgaonkar 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 26: (7 comments) The patch makes sense to me. There is definitely more work needed like updating the lastSyncedEventId from catalogOpExecutor#execDdl. Currently, this patch supports updates to the lastSyncedEventId from CatalogMetastoreServiceHandler and uses it for its self-event detection. It would be good to do the same on the execDdl path in the longer run. I have left some questions/comments which when resolved, I can +2 the patch. 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: EventFactoryForSyncToLatestEvent remove if not needed. http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@919 PS26, Line 919: if (!isSelfEvent || !BackendConfig.INSTANCE.enableSyncToLatestEventOnDdls()) { : return isSelfEvent; : } Not sure if I understand this condition correctly. Why are evaluating old selfEvent when flag is true? Also, it looks like if the old selfEvent evaluation is false, then irrespective of where the syncToLatest flag is true or not we return early. Can we change this to: if (!BackendConfig.INSTANCE.enableSyncToLatestEventOnDdls()) { return isSelfEvent(); } http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@932 PS26, Line 932: tbl.setLastSyncedEventId(getEventId()); Why do we need to set the lastSyncedEventId here? Can we keep the scope of this method to only detecting self-events? I thought we are already setting the lastSyncedEvent when we actually process the method. http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@988 PS26, Line 988: if (!isSelfEvent || !BackendConfig.INSTANCE.enableSyncToLatestEventOnDdls()) { : return isSelfEvent; : } same as previous comment. 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: Reference remove if not needed. 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: // TODO: Should we recreate table if its create event id : // does not match the one passed in fn argument? Is this something that you are working on? http://gerrit.cloudera.org:8080/#/c/17859/26/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@929 PS26, Line 929: // TODO: should event id be set only in case of success? Is this TODO still unresolved? Please remove if it is. My understanding is the it returns false when the Db doesn't exist anymore in which case adding the lastSyncedEventId here doesn't make sense. -- 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: 26 Gerrit-Owner: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sourabh Goyal <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yu-Wen Lai <[email protected]> Gerrit-Comment-Date: Tue, 02 Nov 2021 20:37:07 +0000 Gerrit-HasComments: Yes
