Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20131 )
Change subject: IMPALA-12152: Add query options to wait for HMS events sync up ...................................................................... Patch Set 31: (15 comments) Thanks for Riza's detailed comments! Addressed them. http://gerrit.cloudera.org:8080/#/c/20131/31//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20131/31//COMMIT_MSG@39 PS31, Line 39: catalogd WebUI and metrics > Is it events-processor.lag-time? Can you spell out the metrics name in comm Done http://gerrit.cloudera.org:8080/#/c/20131/31//COMMIT_MSG@84 PS31, Line 84: Currently, UPDATE_TBL_COL_STAT_EVENT, UPDATE_PART_COL_STAT_EVENT, : OPEN_TXN events are ignored by the event processor. If the latest event : happens to be in these types and there are no more other events, the : last synced event id can never reach the latest event id. > Question: Does this mean SYNC_HMS_EVENTS_WAIT_TIME_S will be spent in full Yes. The improvement mentioned below can also help this. http://gerrit.cloudera.org:8080/#/c/20131/30/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/20131/30/be/src/catalog/catalog-server.cc@465 PS30, Line 465: status = catalog_server_->catalog()->WaitForHmsEvent(req, &resp); > OK. Can you put comment in ImpalaService.thrift or somewhere else that is r Yeah, it's not recommended to use them cluster wide. E.g. for queries on tables that are only modified by the same Impala cluster, they don't need these query options since all the HMS events on those tables are self-events and should be skipped. Added more comments in ImpalaService.thrift. http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@73 PS31, Line 73: public String getParsedDb() > Can we add @Nullable annotation here? Done http://gerrit.cloudera.org:8080/#/c/20131/30/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/20131/30/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@4232 PS30, Line 4232: req.timeout_s * 1000L > Ack. Please file a follow up JIRA for that. Sure, filed IMPALA-13685. http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java@46 PS31, Line 46: long getLastSyncedEventId(); > Maybe add default value for new interface method here? Done http://gerrit.cloudera.org:8080/#/c/20131/31/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/20131/31/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1523 PS31, Line 1523: long timeoutMs, Reference<Boolean> success > nit: I think you should create a class two the returned String with success Agree with that. Maybe returning TStatus is simpler so the caller doesn't need to convert the result to TStatus. http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1549 PS31, Line 1549: hmsEventSyncSleepIntervalMs_ > Should this be min(timeoutMs, hmsEventSyncSleepIntervalMs_) ? Yeah. Usually timeoutMs is larger than 1s and hmsEventSyncSleepIntervalMs_ is smaller than 1s (defaults to 100ms). But it's safer to use min() here. http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/20131/31/fe/src/main/java/org/apache/impala/service/Frontend.java@2294 PS31, Line 2294: queryCtx.setWarnings(errorMsg); > Why we need to setWarnings(errorMsg) if errorMsg is already printed in Quer We want to show this error message to the client as well. E.g. [localhost:21050] default> select * from a100; Query: select * from a100 Query submitted at: 2025-01-22 11:14:10 (Coordinator: http://quanlong-Precision-3680:25000) Query state can be monitored at: http://quanlong-Precision-3680:25000/query_plan?query_id=9c416c34b56e05c9:772393ab00000000 +---+---+ | i | p | +---+---+ | 0 | 0 | +---+---+ WARNINGS: Continuing without syncing Metastore events: Timeout waiting for HMS events to be synced. Event id to wait for: 38625. Last synced event id: 38623 Fetched 1 row(s) in 1.12s The warnings are returned in the get_log beeswax RPC or GetLog HS2 RPC. I see there are several ways to add the warnings, e,g, * query_handle->GetAnalysisWarnings() * extract admission control queuing reason from the profile * coord->GetErrorLog() https://github.com/apache/impala/blob/aac375eb200c32a11b94e1e0d986a74737d0636e/be/src/service/impala-hs2-server.cc#L1164-L1183 Puting the warning string in query_ctx and using it in Coordinator::GetErrorLog() seems simpler than extracting it from the profile. http://gerrit.cloudera.org:8080/#/c/20131/31/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/20131/31/tests/common/impala_test_suite.py@383 PS31, Line 383: get_client > Rename to 'default_impala_client'. Done http://gerrit.cloudera.org:8080/#/c/20131/31/tests/common/impala_test_suite.py@1378 PS31, Line 1378: {"sync_hms_events_wait_time_s": timeout_s, "sync_hms_events_strict_mode": True} > Can define a dictionary constant and share this with wait_for_db_to_appear. The value of sync_hms_events_wait_time_s comes from the parameter so we can only add {"sync_hms_events_strict_mode": True} as the constant. Might not save codes. BTW, we need a new client here to avoid resetting existing query options in self.client. Extracted the common code with wait_for_db_to_appear into a new method, execute_query_with_hms_sync(). http://gerrit.cloudera.org:8080/#/c/20131/31/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/20131/31/tests/custom_cluster/test_events_custom_configs.py@1427 PS31, Line 1427: handle = client.execute_async(query) > Close this handle before you run another execute_async at L1444. Done http://gerrit.cloudera.org:8080/#/c/20131/31/tests/metadata/test_event_processing.py File tests/metadata/test_event_processing.py: http://gerrit.cloudera.org:8080/#/c/20131/31/tests/metadata/test_event_processing.py@214 PS31, Line 214: class TestEventSyncWaiting(ImpalaTestSuite): > Override add_test_dimensions so you can define query option that you must h Done. Set the configuration after creating the client since all usages of it share the same query option. http://gerrit.cloudera.org:8080/#/c/20131/31/tests/metadata/test_event_processing_base.py File tests/metadata/test_event_processing_base.py: http://gerrit.cloudera.org:8080/#/c/20131/31/tests/metadata/test_event_processing_base.py@30 PS31, Line 30: def _run_test_insert_events_impl(cls, hive_client, impala_client, impala_cluster, : unique_database, is_transactional=False): > This method modify and clear existing configuration of impala_client. Done. Good point! http://gerrit.cloudera.org:8080/#/c/20131/31/tests/metadata/test_event_processing_base.py@144 PS31, Line 144: def _run_event_based_replication_tests_impl(cls, hive_client, impala_client, : impala_cluster, filesystem_client, transactional=True): > This method modify and clear existing configuration of impala_client. Done. It seems a burden to create and close the impala_client for each method. Maybe we should do these in setup_method() and teardown_method(). -- To view, visit http://gerrit.cloudera.org:8080/20131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ac941bb2c2217b09fcfa2eb567b011b38efa2a Gerrit-Change-Number: 20131 Gerrit-PatchSet: 31 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 22 Jan 2025 08:30:08 +0000 Gerrit-HasComments: Yes
