[email protected] 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 34: (16 comments) http://gerrit.cloudera.org:8080/#/c/21031/33//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21031/33//COMMIT_MSG@20 PS33, Line 20: Existing metastore event processing is turned into multi-level : event processing with enable_hierarchical_event_processing flag. > Please add short paragraph describing the relationship between DBEventExecu Added http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc@222 PS33, Line 222: num_db_event_execu > nit: num_db_event_executors sounds better. Done http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc@230 PS33, Line 230: num_table_event_execu > nit: num_table_event_executors_per_db sounds better. Done http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc@237 PS33, Line 237: min_event_processor_idle_m > nit: Since ses_ is scheduleAtFixedRate, can you rename this flag to reflect Have changed it to time period now. min_event_processor_idle_ms is the minimum time to retain idle db processors and table processors on the database event executors and table event executors respectively, when they do not have events to process. Idle db processors and table processors that have elapsed this time are purged later. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@45 PS33, Line 45: > nit: Please consistently use DBEventExecutor in documentation. It helps rea Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@53 PS33, Line 53: > Is the number of TableEventExecutor per DBEventExecutor is fixed all the ti Yes. Updated the description. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@71 PS33, Line 71: > Move the comment above the field declaration. What is an example/pattern of Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@73 PS33, Line 73: > Please use more descriptive variable name. Maybe service_? Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@91 PS33, Line 91: > usableLock_ seems to protect more than just usable_. Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@125 PS33, Line 125: * segregated from the input event queue as database events and table events. Table : * events are dispatched to the a > usableLock_ seems to protect more than just usable_. Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@231 PS33, Line 231: } : } : > setUsable(false); This the only place DBProcessor usable_ is set to false. IMO, we don't need to have an explicit setUsable method for it. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@439 PS33, Line 439: "Executor: {} might have been cleared. Not decrementing outstanding event count", : name_); > Please write short documentation for this method and others that does not h Done http://gerrit.cloudera.org:8080/#/c/21031/33/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/33/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@661 PS33, Line 661: .build()); : > I think you should wrap this list of dbEventExecutors_ and its associated m Done. Have added EventExecutorService. Have added the EventExecutorServiceTest to 1. Test assignment, unassignment of executors to databases and tables. 2. Test start, clear, stop and process metastore events processing in hierarchical mode. http://gerrit.cloudera.org:8080/#/c/21031/33/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/33/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@53 PS33, Line 53: TableEvent > nit: Fully qualified table name Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@55 PS33, Line 55: > Move the comment above the field declaration. What is an example/pattern of Done http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@57 PS33, Line 57: > Please use more descriptive variable name. Maybe service_? Done -- 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: 34 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: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 21 Jan 2025 19:19:17 +0000 Gerrit-HasComments: Yes
