Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20533 )
Change subject: IMPALA-12463: Batch non-consecutive table events in the event processor ...................................................................... Patch Set 8: Code-Review+1 (2 comments) Left some nitpicks, otherwise the code LGTM! http://gerrit.cloudera.org:8080/#/c/20533/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/20533/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@391 PS8, Line 391: // drop database - cuts any existing batches for tables in that database : // alter table rename - cuts any existing batches for the source or destination : // table : // alter database - cuts any existing batches for tables in the database : // This should be rare, so the performance difference is minimal. nit: the comments could be in the order as they appear in the code, i.e.: * drop database ... * alter database ... * alter table rename ... http://gerrit.cloudera.org:8080/#/c/20533/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@396 PS8, Line 396: if (next instanceof DropDatabaseEvent || next instanceof AlterDatabaseEvent) { : // Any batched events for tables from this database need to be flushed : // before emitting the AlterDatabaseEvent or DropDatabaseEvent. : flushBatchesForDb(pendingTableEventsMap, sortedFinalBatches, next.getDbName()); : } else if (next instanceof AlterTableEvent) { : AlterTableEvent alterTable = (AlterTableEvent) next; : if (alterTable.isRename()) { : // Flush any batched events for the source table. : Table beforeTable = alterTable.getBeforeTable(); : flushBatchForTable(pendingTableEventsMap, sortedFinalBatches, beforeTable); : : // Flush any batched events for the destination table. Given that the : // destination table must not exist when doing this rename, valid sequences : // are already handled implicitly by the existing batch-breaking logic : // (combined with the sorting of the final batches). This does the flushing : // explicitly in case there are any edge cases not handled by the existing : // mechanisms. : Table afterTable = alterTable.getAfterTable(); : flushBatchForTable(pendingTableEventsMap, sortedFinalBatches, afterTable); : } : } nit: for readability, would it make sense to put these if statements into their own methods? E.g. what we could have here is just two method calls: cutDbEventsIfNeeded(next, pendingTableEventsMap, sortedFinalBatches); cutTableEventsIfNeeded(next, pendingTableEventsMap, sortedFinalBatches); -- To view, visit http://gerrit.cloudera.org:8080/20533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I849c0306bc46080ee4059854f42d9c217a89b905 Gerrit-Change-Number: 20533 Gerrit-PatchSet: 8 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 22 Dec 2023 18:03:05 +0000 Gerrit-HasComments: Yes
