Joe McDonnell 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: (2 comments) 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.: Good idea, done 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 t It does make sense to split this logic out into its own function. That provides a better place to describe why we are doing this. I split this out to be its own "cutBatchesForCrossTableEvents" function. -- 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 <joemcdonn...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Jan 2024 18:20:17 +0000 Gerrit-HasComments: Yes
