Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/19484 )
Change subject: IMPALA-11822: Optimize the Refresh/Invalidate event processing by skipping unnecessary events ...................................................................... Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/19484/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19484/6//COMMIT_MSG@10 PS6, Line 10: the : latest event id before loading the table/partition. : This will be up > nit: "the latest event id before loading the table/partition" ? Ack http://gerrit.cloudera.org:8080/#/c/19484/6//COMMIT_MSG@17 PS6, Line 17: 's event id, invalidate event anyway flus > nit: this seems belonging to the previous patch. No map now. Ack http://gerrit.cloudera.org:8080/#/c/19484/6//COMMIT_MSG@21 PS6, Line 21: optimization to work: > nit: these need to be updated Ack http://gerrit.cloudera.org:8080/#/c/19484/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/19484/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2572 PS5, Line 2572: if (currentHmsEventId > eventId) { > Yeah, the new change LGTM. BTW, it seems they are the same solution, i.e. i If currentHMSEventId is -1, then I'll set current eventId as latestEventID. This is useful in cases where enableSyncToLatestEventOnDdls is set to false (instead of setting latesteventid to -1, I would like to set it to current event id). http://gerrit.cloudera.org:8080/#/c/19484/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3847 PS5, Line 3847: i > Yeah, we should change the name, otherwise it's misleading. I think somethi Ack http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@750 PS6, Line 750: happene > nit: "happened" Ack http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2883 PS6, Line 2883: LOG.warn(String.format("Unable to fetch latest event id from HMS: %s", > Let's tolerate the error since this is just for optimization. I think repla Ack http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/Table.java@1033 PS6, Line 1033: > nit: please keep only one blank line between methods Ack http://gerrit.cloudera.org:8080/#/c/19484/6/fe/src/main/java/org/apache/impala/catalog/Table.java@1039 PS6, Line 1039: } > Should we check if eventId is -1 before this? i.e. only update lastRefreshE Ack http://gerrit.cloudera.org:8080/#/c/19484/6/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/19484/6/tests/custom_cluster/test_events_custom_configs.py@236 PS6, Line 236: enable_sync_to_latest_event_on_ddls > If the optimization depends on this flag, we should mention it in the commi Ack -- To view, visit http://gerrit.cloudera.org:8080/19484 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I905957683a96c3ea01ab4bf043d6658ce37b7574 Gerrit-Change-Number: 19484 Gerrit-PatchSet: 7 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 08 Mar 2023 15:12:25 +0000 Gerrit-HasComments: Yes
