Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20367 )
Change subject: IMPALA-10976: Sync db/table to latest HMS event for all DDL/DMLs ...................................................................... Patch Set 38: (7 comments) http://gerrit.cloudera.org:8080/#/c/20367/38/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/20367/38/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@924 PS38, Line 924: false nit: need a comment of "/*isSkipTableMetadataReload*/" to be consistent with the existing code style. http://gerrit.cloudera.org:8080/#/c/20367/38/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/20367/38/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@537 PS38, Line 537: case ALTER_PARTITIONS: Could you share how we will go here? NotificationEvent "event" comes from HMS and we convert the event type in MetastoreEventType.from(). Does HMS really have an event type of "ALTER_PARTITIONS"? BTW, shouldn't we use BatchPartitionEvent<AlterPartitionEvent> if we really need this case? http://gerrit.cloudera.org:8080/#/c/20367/38/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1792 PS38, Line 1792: !Objects.equals(beforeSd.getSerdeInfo(), afterSd.getSerdeInfo())) { I'm thinking if we could split these into a separate patch for IMPALA-12487 so it can be easier to backport these to older branches. What do you think? http://gerrit.cloudera.org:8080/#/c/20367/38/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/20367/38/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@390 PS38, Line 390: for (NotificationEvent event : events) { : currentEvent = eventFactory.get(event, metrics); Shouldn't we use 'filteredEvents' instead? http://gerrit.cloudera.org:8080/#/c/20367/38/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/20367/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1139 PS38, Line 1139: Hdfs table nit: "Hdfs table" might be misunderstood as the real table on HDFS. Maybe using 'tbl' here is more specific, i.e. 'tbl' will not be updated in alterTableAddPartitions() if... http://gerrit.cloudera.org:8080/#/c/20367/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2644 PS38, Line 2644: to keep file metadata up-to-date We won't reload the file metadata now since we have reverted the changes in "file_metadata_reload_properties". Do we still need this? http://gerrit.cloudera.org:8080/#/c/20367/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3230 PS38, Line 3230: or syncToLatestEventId is enabled Could you turn this into a TODO comment with the Hive JIRA for skip dropping stats in truncateTable()? -- To view, visit http://gerrit.cloudera.org:8080/20367 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia250d0a943838086c187e5cb7c60035e5a564bbf Gerrit-Change-Number: 20367 Gerrit-PatchSet: 38 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Mon, 05 Feb 2024 01:37:33 +0000 Gerrit-HasComments: Yes
