Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19378 )
Change subject: IMPALA-11808: Add support for reload event in catalogD ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/19378/8/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/19378/8/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@429 PS8, Line 429: So empty list is returned : * : * @param msClient Metastore client, : * @param isRefresh if this flag is set to true then it is a refresh query, else it : * is an invalidate metadata query. : * @param partVals The partition list corresponding to : * the table, used by Apache Hive 3 : * @param dbName : * @param tableName : * @return a list of eventIds for the reload events These comments are stale now. http://gerrit.cloudera.org:8080/#/c/19378/8/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/19378/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@541 PS8, Line 541: updateFieldsForReloadEvent nit: "getFieldsFromReloadEvent" sounds better since it returns the results but not updates anything. http://gerrit.cloudera.org:8080/#/c/19378/8/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/19378/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6417 PS8, Line 6417: MetastoreShim.fireReloadEventHelper(catalog_.getMetaStoreClient(), Just realized that the returned event ids are ignored. Does the self-event check of reload events depend on the event id, or depend on catalog version? I see we are passing event ids to the SelfEventContext https://gerrit.cloudera.org/c/19378/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java#2466 I'm not that clear how the self-events are skipped in the test. I might miss something.. http://gerrit.cloudera.org:8080/#/c/19378/8/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/19378/8/tests/custom_cluster/test_events_custom_configs.py@246 PS8, Line 246: def check_self_events(query): Can we add an optional argument for the partition values (default to None) and check it? -- To view, visit http://gerrit.cloudera.org:8080/19378 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic62d58837d356dc2113f3c0904228ac9de484136 Gerrit-Change-Number: 19378 Gerrit-PatchSet: 8 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[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: Thu, 05 Jan 2023 01:13:47 +0000 Gerrit-HasComments: Yes
