Csaba Ringhofer 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 20: (8 comments) http://gerrit.cloudera.org:8080/#/c/21031/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21031/20//COMMIT_MSG@35 PS20, Line 35: Testing: Did you try running the full test suite while turning hierarchical event processing on by default? While some event processing related tests would probably fail, most tests should be green. 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 Instead of adding a new flag the old hms_event_polling_interval could be changed double, for example 1500ms could be expressed as 1.5. Even if the current new flag is kept, the old flags's description should be extended with info about the new flag. http://gerrit.cloudera.org:8080/#/c/21031/20/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/20/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@288 PS20, Line 288: public void cleanup() { cleanup(false); } : : public synchronized void clear() { I am confused about locking - only a few public functions are synchronized - does that mean that other functions are safe to call in parallel? For example can someone call cleanup() while process() is running? http://gerrit.cloudera.org:8080/#/c/21031/20/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/20/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@832 PS20, Line 832: MetastoreEvents.MetastoreEventType eventType = event.getEventType(); can you add some comments about the role of the pseudo events? http://gerrit.cloudera.org:8080/#/c/21031/20/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@863 PS20, Line 863: // Enqueue a drop table event to old db event executor and a create table event to : // new db event executor I am not completely convinced that this is a good idea. Processing the drop/create on different threads means that the users may see both the new and the old version at the same time transiently or none of them. http://gerrit.cloudera.org:8080/#/c/21031/20/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@882 PS20, Line 882: pseudoCreateTableEvent.setTableName(alterEvent.getBeforeTable().getTableName()); : pseudoCreateTableEvent.setDbName(alterEvent.getBeforeTable().getDbName()) Shouldn't these use AfterTable instead of BeforeTable? http://gerrit.cloudera.org:8080/#/c/21031/20/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@885 PS20, Line 885: pseudoCreateTableEvent.setEventId(event.getEventId()); Can't it confuse other parts of the code if to events have the same event id? http://gerrit.cloudera.org:8080/#/c/21031/20/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/20/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@223 PS20, Line 223: tableProcessors_ Do I understand correctly that the tableProcessors_ will have a fixed order, and the events will be processed in this order instead of event id order? Does this also apply to DROP/CREATE table events? In case of renames, what guarantees that the CREATE event won't be processed before the DROP event if the the new table name is earlier among the table processors? -- 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: 20 Gerrit-Owner: 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, 09 Jul 2024 17:00:34 +0000 Gerrit-HasComments: Yes
