Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21031 )
Change subject: IMPALA-12709: Add support for hierarchical metastore event processing ...................................................................... Patch Set 27: (7 comments) http://gerrit.cloudera.org:8080/#/c/21031/27//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21031/27//COMMIT_MSG@72 PS27, Line 72: - Executed existing end to end tests. Please mention FE tests need some changes to run on the new mode. Maybe after we add the SQL functionality to stop/resume event processing (IMPALA-12785), we can move tests in MetastoreEventsProcessorTest to e2e tests. Currently, skipping FE tests is OK to me since the new mode is turned off by default. http://gerrit.cloudera.org:8080/#/c/21031/20/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21031/20/be/src/catalog/catalog-server.cc@217 PS20, Line 217: hms_event_polling_interval_ms > For backward compatility, have not modified the existing flag. Have updated The startup flags are used as text, e.g. in scripts or configuration files. I think changing the type of 'hms_event_polling_interval_s' to double won't break anything. http://gerrit.cloudera.org:8080/#/c/21031/27/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21031/27/be/src/catalog/catalog-server.cc@243 PS27, Line 243: DEFINE_int32(table_event_executors, 5, For testing purpose, we might need one more flag to control the max number of db/table processors in a single Db/TableEventExecutor. So we can test the scenario of events of different tables are processed in different threads by setting that flag to 1. Then we can test the synchronization of DBBarrierEvent and RenameTableBarrierEvent. http://gerrit.cloudera.org:8080/#/c/21031/27/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/21031/27/common/thrift/metrics.json@3616 PS27, Line 3616: "key" : "events-processor.lag-time" We need to redefine what the lag is in the new mode. Or maybe add more metrics like how many tables have pending events and the max lag of them. I'm OK to do this in future changes. Please add TODOs and mention it in the commit message. http://gerrit.cloudera.org:8080/#/c/21031/27/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/21031/27/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1275 PS27, Line 1275: } catch (Exception ex) { handleEventProcessException(ex); } finally { nit: please don't wrap this. I don't think we have existing codes in this style. http://gerrit.cloudera.org:8080/#/c/21031/27/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1583 PS27, Line 1583: LOG.info("Time elapsed in processing event batch: {}", Please add TODOs to improve/fix these logs in the new mode, and mention it in the commit message. http://gerrit.cloudera.org:8080/#/c/21031/27/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/21031/27/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@139 PS27, Line 139: event.debugLog("Table barrier reached for table: {}", fqTableName_); Since TableEventExecutor#process() is scheduled every 10ms, we need to avoid flushing lots of such logs. E.g. when the DROP event is waiting for some previous events to finish. Maybe log it just at the first time, or only log it at 100th occurence so the log appears every 1s. -- To view, visit http://gerrit.cloudera.org:8080/21031 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76d8a739f9db6d40f01028bfd786a85d83f9e5d6 Gerrit-Change-Number: 21031 Gerrit-PatchSet: 27 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[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: Tue, 10 Dec 2024 07:32:28 +0000 Gerrit-HasComments: Yes
