Sai Hemanth Gantasala 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 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/20367/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20367/7//COMMIT_MSG@7 PS7, Line 7: IMPALA-10976: Sync db/table to latest HMS event for all DDL/DMLs : > nit: Let's put the title in one line I thought the commit message limit was 52 characters. Let me correct it. http://gerrit.cloudera.org:8080/#/c/20367/7//COMMIT_MSG@29 PS7, Line 29: et 'file_metadata_reload_pro > How does this flag impact the feature added by this patch? I think it only https://github.com/apache/impala/blob/master/be/src/catalog/catalog-server.cc#L112 -- This flag is set to true by default. https://github.com/apache/impala/blob/master/be/src/catalog/catalog-server.cc#L125 -- If enable_sync_to_latest_event_on_ddls is enabled we should disable invalidate_hms_cache_on_ddls flag. http://gerrit.cloudera.org:8080/#/c/20367/7//COMMIT_MSG@31 PS7, Line 31: : Note: We don't modify the cache using MetastoreEventsProcessor for : alter table rename > Please file a follow-up JIRA for this. Done. https://issues.apache.org/jira/browse/IMPALA-12553 Should I mention the jira in the commit message also? http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@147 PS7, Line 147: at leas > nit: please fix this typo BTW - "at least" Ack http://gerrit.cloudera.org:8080/#/c/20367/7/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/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6666 PS7, Line 6666: req.getTable_name(), tblWasRemoved, dbWasAdded); > This seems redundant. I need to use this in L#6673 - fireReloadEventAndUpdateRefreshEventId(). Let me move this inside if condition. http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6667 PS7, Line 6667: } : } : } else { > I'm not sure if we should enforce this. Intuitively we can reload the table https://gerrit.cloudera.org/c/20367/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#7049 -- Here you have suggested that we should use MetastoreEventsProcessor to update the cache, if we want to do that when enable_sync_ddl is on, we should enforce the precondition that enable_reload_events is set to true, fire the reload to HMS, and consume the same event via MetaStoreEventsProcessor to update the cache instead of updating it in place. In the previous patch, I was rooting for not to implement this feature for the same reason since there is no actual modification in HMS. http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6690 PS7, Line 6690: if (!req.isIs_refresh()) { > When syncToLatestEventId is on, can we add a warning saying events on the t Partition-level refresh events are updated in place in the cache. Here is the test https://gerrit.cloudera.org/c/20367/7/fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsSyncToLatestEventIdTest.java#415 http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7745 PS7, Line 7745: : > nit: remove such empty comments Ack http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/20367/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@3495 PS7, Line 3495: */ : public TDdlExecRequest getDropTableFromImpalaReq(String dbName, String tblName) : throws ImpalaException { : TDdlExecRequest req = ne > nit: please remove these actually empty comments Ack. Corrected others as well -- 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: 9 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: Wed, 29 Nov 2023 03:29:35 +0000 Gerrit-HasComments: Yes
