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 9: (15 comments) 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: getEventProcessor(); > Done Done http://gerrit.cloudera.org:8080/#/c/22997/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@682 PS9, Line 682: } catch (EventProcessException e) { : LOG.error("Exception occurred for executor: {}", name_); : eventProcessor_.handleEventProcessException(e.getException(), e.getEvent()); : } What happen if other Exception happen (not an EventProcessException)? How to handle them? 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: // In-progress event log. Maintains the metastore events waiting to be processed on : // executors. It is a map of metastore event id to the pair of metastore event and : // whether it is del > Done Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@86 PS6, Line 86: ng. The delimite > Done Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@178 PS6, Line 178: > Done Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@449 PS6, Line 449: // Reinitialize with the current > We use the same field to determine the time taken for event dispatch and ev I strongly suggest using different field for different purpose. See my reason here: https://gerrit.cloudera.org/c/22997/9#637 http://gerrit.cloudera.org:8080/#/c/22997/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@441 PS9, Line 441: dispatch time "time to dispatch" is better. http://gerrit.cloudera.org:8080/#/c/22997/9/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@502 PS9, Line 502: private void pruneProcessedLog() Mark this method synchronized because processedLog_ is mutated below. Calling synchronized method from another synchronized method should be fine. 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_; > We use the same field to determine the time taken for event dispatch and ev I strongly suggest using different field for different purpose. See my reason here: https://gerrit.cloudera.org/c/22997/9#637 http://gerrit.cloudera.org:8080/#/c/22997/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@628 PS9, Line 628: // To record the current time in milliseconds. It can be used to calculate the time : // taken for the particular operation like, event dispatch time, processing : // time etc. It is initialized when MetastoreEvent object is created. And it is reset : // with new time at the end of dispatch after logging the dispatch time. i.e., time : // taken from actual MetastoreEvent object creation until all its associated : // DerivedMetastoreEvent are dispatched. A log is added after processing the event to : // show the processing time. i.e., time taken after dispatch until the actual : // MetastoreEvent (i.e., all its associated DerivedMetastoreEvent) is processed. : // This field is not used for DerivedMetastoreEvent. : protected long recordTime_; The code should be self explanatory. Reusing 1 field for 2 different purpose will cause potential bug in the future if other contributor does not read this documentation. Please separate it to 2 different fields: creationTime_ and dispatchTime_. Please guarantee that once they are set, they can not be mutated again. 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: > Done Done http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@249 PS6, Line 249: e void postProcessEvent(MetastoreEve > We can have DbBarrierEvent that need to ignored. Have refactored now such t New code in patch set 9 looks OK. Thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/22997/6/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@472 PS6, Line 472: for (Map.Entry<String, TableProcessor> entry : tableProcessors_.entrySet()) { : TableProcessor tableProcessor = entry.getValue(); : if (!eventProcessor_.canProcessEventInCurrentStatus()) break; : > Done Done http://gerrit.cloudera.org:8080/#/c/22997/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@330 PS9, Line 330: if (!isProcessed) return; Please remind me, what happen in this situation? Is the event retried or discarded? http://gerrit.cloudera.org:8080/#/c/22997/9/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@477 PS9, Line 477: } catch (EventProcessException e) { : LOG.error("Exception occurred for executor: {}", name_); : eventProcessor_.handleEventProcessException(e.getException(), e.getEvent()); : } What happen if other Exception happen (not an EventProcessException)? How to handle them? -- 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: 9 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: Tue, 24 Jun 2025 15:45:19 +0000 Gerrit-HasComments: Yes
