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 36: (12 comments) http://gerrit.cloudera.org:8080/#/c/20131/35//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20131/35//COMMIT_MSG@45 PS35, Line 45: A succeeded wait: : Query Compilation: 937.279ms : - Synced events from Metastore: 909.162ms (909.162ms) : - Metadata of all 1 tables cached: 911.005ms (1.843ms) : - Analysis finished: 919.600ms (8.595ms) : : A failed wait: : Query Compilation: 1s321ms : - Continuing without syncing Metastore events: 40.883ms (40.883ms) : - Metadata load started: 41.618ms (735.633us) > Any test validating existence of new event timeline in query profile? TestEventSyncFailures validates "Continuing without syncing Metastore events" in the profile: https://gerrit.cloudera.org/c/20131/35/tests/custom_cluster/test_events_custom_configs.py Extended it to validate the labels in the timeline. Also added a positive test for label "Synced events from Metastore" in TestEventSyncWaiting.test_hms_event_sync. http://gerrit.cloudera.org:8080/#/c/20131/35//COMMIT_MSG@85 PS35, Line 85: - Note that we still need wait_for_event_processing() in test codes : that > Unrelated question: Do you know if updating table property will raise HMS e Yes, when you want to update a table property, you need to use the alter_table HMS RPC which will generate an ALTER_TABLE event containing the original and new msTbl objects. Catalogd will reload the table when processing the ALTER_TABLE event. https://github.com/apache/impala/blob/1f7b9601e5a768c0b2061fef95c750ae74059b84/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java#L1848 http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/catalog/catalog-server.cc@103 PS35, Line 103: used in the > Who is waiting? Maybe tell us something about this flag in commit message? The waiting thread in catalogd: https://gerrit.cloudera.org/c/20131/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java#3816 Added more description. http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/catalog/catalog-server.cc@471 PS35, Line 471: : void WaitForHmsEvent(TWaitForHmsEventResponse& res > Please add gauge / metric on how much WaitForHmsEvent RPC is currently hung Done. Added a histogram metric, impala-server.wait-for-hms-event-durations-ms http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/service/impala-server.cc@2322 PS35, Line 2322: << " current version: " << catalog_update_info_.catalog_version; : while (catalog_update_info_.catalog_version < catalog_update_version && : > This pattern if timeline != nullptr check followed by timeline->MarkEvent() Yeah, that looks better. http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/20131/35/be/src/service/query-options.h@367 PS35, Line 367: QUERY_OPT_FN(sync_hms_events_wait_time_s, SYNC_HMS_EVENTS_WAIT_TIME_S, \ : TQueryOptionLevel::ADVANCED) \ : QUERY_OPT_FN(sync_hms_events_strict_mode, SYNC_HMS_EVENTS_STRICT_MODE, \ : TQueryOptionLevel::ADVANCED) \ > Is this strict for DEVELOPMENT / testing, or do you envision this to become I thought DEVELOPMENT means not GA yet. It seems ADVANCED is more appropriate. I'll file a doc JIRA once this is merged. http://gerrit.cloudera.org:8080/#/c/20131/35/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/35/fe/src/main/java/org/apache/impala/service/Frontend.java@2190 PS35, Line 2190: private static TCatalogServiceRequestHeader createCatalogServiceRequ > This can be static? createCatalogServiceRequestHeader sounds better. Done http://gerrit.cloudera.org:8080/#/c/20131/35/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/20131/35/tests/common/impala_test_suite.py@1429 PS35, Line 1429: progress is None: > make this return result? Ah, yeah. We should do this. http://gerrit.cloudera.org:8080/#/c/20131/35/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/20131/35/tests/custom_cluster/test_events_custom_configs.py@436 PS35, Line 436: execute_query_ > Why not use the new execute_query_with_hms_sync? Extended execute_query_with_hms_sync to support non-strict mode so we can use it. http://gerrit.cloudera.org:8080/#/c/20131/35/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/20131/35/tests/custom_cluster/test_kudu.py@323 PS35, Line 323: execute_query > Why not use the new execute_query_with_hms_sync? Modified execute_query_with_hms_sync to invoke execute_query() instead of execute_query_expect_success() so we can use it here. http://gerrit.cloudera.org:8080/#/c/20131/35/tests/metadata/test_event_processing.py File tests/metadata/test_event_processing.py: http://gerrit.cloudera.org:8080/#/c/20131/35/tests/metadata/test_event_processing.py@293 PS35, Line 293: @SkipIfCatalogV2.hms_event_polling_disabled() > hms_event_polling is enabled by default, right? Yeah, just to be consistent with other EventProcessor tests. If you launch the cluster with EventProcessor disabled and run e2e tests, these tests will be skipped. http://gerrit.cloudera.org:8080/#/c/20131/35/tests/metadata/test_event_processing_base.py File tests/metadata/test_event_processing_base.py: http://gerrit.cloudera.org:8080/#/c/20131/35/tests/metadata/test_event_processing_base.py@30 PS35, Line 30: def _run_test_insert_events_impl(cls, unique_database, is_transactional=False): > Except for EventProcessorUtils.wait_for_event_processing_impl removal in fa Yeah, it's due to the with-clause. It seems after IMPALA-13694, we don't need to create a new impala client anymore. -- 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: 36 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: Fri, 14 Feb 2025 12:18:57 +0000 Gerrit-HasComments: Yes
