[email protected] 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: (10 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 > Can this just pass context as param? 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; > At constructor, I think it is better to assign from constructor parameter a Done http://gerrit.cloudera.org:8080/#/c/22997/4/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/4/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@335 PS4, Line 335: MetastoreTableEvent dropTableEvent = new DropTableEvent( : alterEvent.getCatalogOpExecutor(), alterEvent.getMetrics(), pseudoDropTableEvent, : alterEvent.getBeforeTable()); > What is the recordTime_ of dropTableEvent? It is initialized with System.currentTimeMillis(). It will not be used since it is part of RenameTableBarrierEvent(derived event). http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@345 PS4, Line 345: MetastoreTableEvent createTableEvent = new CreateTableEvent( : alterEvent.getCatalogOpExecutor(), alterEvent.getMetrics(), : pseudoCreateTableEvent, alterEvent.getAfterTable()); > What is the recordTime_ of createTableEvent? It is initialized with System.currentTimeMillis(). It will not be used since it is part of RenameTableBarrierEvent(derived event). http://gerrit.cloudera.org:8080/#/c/22997/2/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/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@628 PS2, 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 > Not addressed yet. If remain unchanged, please clarify the documentation to Have updated the doc. It is always initialized for actual MetastoreEvent. And it is not used for DerivedMetastoreEvent(value is 0). http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1023 PS2, Line 1023: / Marks the derived event as processed. > Not addressed yet. Done http://gerrit.cloudera.org:8080/#/c/22997/4/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/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@646 PS4, Line 646: // certain event types in Hive-3 like COMMIT_TXN may not have dbName set : this.catalogName_ = event.getCatName(); : t > Consider changing DerivedMetastoreEvent interface into an abstract class th This record time is not applicable for DerivedMetastoreEvent. It is applicable only for actual MetastoreEvent. http://gerrit.cloudera.org:8080/#/c/22997/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@652 PS4, Line 652: if (!(this instance > Consider adding precondition to verify that this never return 0. Have updated code and documentation about the usage of recordTime_. It is 0 for DerivedMetastoreEvent. http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/22997/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1019 PS2, Line 1019: LOG.info( : String.format("Starting metastore even > Where is this set? I do not see pauseGracefully() propagate to pause() anyw pause() is first line in pauseGracefully(). 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: > What is the recordTime_ of RenameTableBarrierEvent itself? Have removed this now. Have updated the code and documentation about the usage of recordTime_ in MetastoreEvent. recordTime_ is applicable only for actual MetastoreEvent. It is 0 for DerivedMetastoreEvent. -- 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 <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 20 Jun 2025 13:25:07 +0000 Gerrit-HasComments: Yes
