Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22997 )
Change subject: IMPALA-13801: Support greatest synced event with hierarchical metastore event processing ...................................................................... Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@1043 PS4, Line 1043: super(context); : txnId_ = ((CommitTxnEvent) context.getActualEvent()).txnId_; : writeIdsInCat > Done Done http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@1046 PS4, Line 1046: writeIdsInEvent_ = writeIdsInEvent; > Done Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@269 PS6, Line 269: dbEventExecutor_.eventProcessor_ This is duplicated in multiple places. Consider making getter method for it. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java File fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java: http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@81 PS6, Line 81: // Dispatched events tree map. Maintains the events waiting to be processed on executors : private final TreeMap<Long, Pair<MetastoreEvent, Boolean>> dispatchedEvents_ = : new TreeMap<>(); dispatchedEvents_ might be interpreted as finished/done. I'd suggest renaming it to inProgressLog_. Please also explain in the doc what the Boolean attribute is. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@86 PS6, Line 86: processedEvents_ Consider renaming this to processedLog_. Please also explain in the doc what the key and value is. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@178 PS6, Line 178: clearInternal Rename this to clearLogs? http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@449 PS6, Line 449: event.setRecordTime(currentTime); This seems the only place setRecordTime() is called. Consider renaming it to setDispatchedTime(). http://gerrit.cloudera.org:8080/#/c/22997/6/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/22997/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@637 PS6, Line 637: protected long recordTime_; Consider renaming it to dispatchedTime_. It will be great to limit the usage such that it is only valid to set it once. http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/main/java/org/apache/impala/catalog/events/RenameTableBarrierEvent.java File fe/src/main/java/org/apache/impala/catalog/events/RenameTableBarrierEvent.java: http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/main/java/org/apache/impala/catalog/events/RenameTableBarrierEvent.java@150 PS4, Line 150: > Have removed this now. Have updated the code and documentation about the us Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@179 PS6, Line 179: tableEventExecutor_.eventProcessor_ This is duplicated in multiple places. Consider making getter method for it. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@249 PS6, Line 249: event instanceof MetastoreTableEvent Should this be a Precondition? Looks like event is always MetastoreTableEvent here. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@472 PS6, Line 472: } catch (Exception e) { : LOG.error("Exception occurred for executor: {}", name_); : eventProcessor_.handleEventProcessException(e, currentEvent_); : } Can you create EventProcessException class and embed the failing event into that exception class? That way, you don't need to have currentEvent_ field and call setCurrentEvent() every time. -- To view, visit http://gerrit.cloudera.org:8080/22997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26240f36aaf85125428dc39a66a2a1e4d3197e85 Gerrit-Change-Number: 22997 Gerrit-PatchSet: 6 Gerrit-Owner: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@gmail.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: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Jun 2025 03:57:12 +0000 Gerrit-HasComments: Yes
