[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/23789 )
Change subject: IMPALA-14535: Improve wait for HMS events sync with hierarchical event processing ...................................................................... Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/23789/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23789/2//COMMIT_MSG@11 PS2, Line 11: . > nit: add space Done http://gerrit.cloudera.org:8080/#/c/23789/2/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/23789/2/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@476 PS2, Line 476: nts with event ids less tha > nit: I think we also ensure the given event id is processed here. Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@483 PS2, Line 483: // Return false if the DbProcessor is being terminated. May happen when event > nit: Please add a comment for when this will happen. If an idle DbProcessor isTerminating() is true when EventProcessor is being paused or stopped. Have add a comment. http://gerrit.cloudera.org:8080/#/c/23789/2/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/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1912 PS2, Line 1912: try { : latestEventId = getCurrentEventId(); : } catch (Meta > This will fail the query. Shouldn't we add a warning and use latestEventId Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1969 PS2, Line 1969: /** > nit: we tend to not align method parameters in Java. Just add 4 more whites Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1976 PS2, Line 1976: MetadataWithHierarchic > nit: It'd be less misleading to use a new method name like maxDispatchedEve Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1981 PS2, Line 1981: long sleepIntervalMs = Math.min(timeoutMs, hmsEventSyncSleepIntervalMs_); > nit: I think this is just a subset of all dbs since idle DbProcessors will Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@2006 PS2, Line 2006: it.remove(); : // Gets the list of tables that have recently received/processed metastore : // events for the db and are not yet purged by idle processor cleanup. : List<String> tableNames = eventExecutorService_.getTableNames(dbName); : if (!tableNames.isEmpty()) db2Tables.put(dbName, tableNames); : } : } : Uninterruptibles.sleepUninterruptibly(sleepIntervalMs, TimeUnit.MILLISECONDS); : } : } : // Wait till necessary dbs and tables are synced : boolean isSyncInProgress = !dbNames.isEmpty() || !db2Tables.isEmpty(); : whi > nit: extract these into a method and invoke it once before the loop to avoi Since it is used only at this place, have refactored it instead. Moved the sleep to bottom of the loop and sleep is based on condition that dbNames or db2Tables is not empty. http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@2050 PS2, Line 2050: .join(db2Tables)); > nit: Please add a Precondition check that isHierarchicalEventProcessingEnab Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@2086 PS2, Line 2086: : Check transactio > nit: might be more clear to mention we are getting names from the request, Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@2087 PS2, Line 2087: // Such events (COMMIT_TXN, > nit: shorten the indentation Done http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java File fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java: http://gerrit.cloudera.org:8080/#/c/23789/2/fe/src/test/java/org/apache/impala/catalog/events/EventExecutorServiceTest.java@202 PS2, Line 202: | > Should we use "|=" here? Yes. That was a mistake. Have fixed it. http://gerrit.cloudera.org:8080/#/c/23789/2/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/23789/2/tests/custom_cluster/test_events_custom_configs.py@2005 PS2, Line 2005: client.wait_for_finished_timeout(handle, 5) > Let's also verify the table names when IMPALA_TEST_CLUSTER_PROPERTIES.is_hi Done -- To view, visit http://gerrit.cloudera.org:8080/23789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55cea4cb8e04860202e56e1b1bf2596613b4946c Gerrit-Change-Number: 23789 Gerrit-PatchSet: 5 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[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, 03 Feb 2026 05:18:13 +0000 Gerrit-HasComments: Yes
