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 38: (4 comments) http://gerrit.cloudera.org:8080/#/c/20131/37/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20131/37/be/src/service/impala-server.cc@2352 PS37, Line 2352: > Repeated string. This can be a constant and reused. Good point! I think adding the method info might be better, e.g. "Detected catalog service ID changed in waiting for catalog propagation". http://gerrit.cloudera.org:8080/#/c/20131/37/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/20131/37/common/thrift/CatalogService.thrift@788 PS37, Line 788: // Set to true for SHOW TABLES/VIEWS statements. > typo: statements Done http://gerrit.cloudera.org:8080/#/c/20131/37/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/37/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@4200 PS37, Line 4200: String dbName, String tblName) { > nit: This method seems like it should be two methods, either taking tbl or Yeah, dbName and tblName are only used when tbl == null. Initially, I just want to wrap the null check into a method. After some refactors, it seems using two methods is more clear. Extracted the code dealing with tbl == null into a new method, addRemovedTableToWaitForHmsEventResponse. http://gerrit.cloudera.org:8080/#/c/20131/37/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/20131/37/tests/custom_cluster/test_events_custom_configs.py@1512 PS37, Line 1512: client.set_configuration({"sync_hms_events_wait_time_s": 10}) > Does this wait for 60 seconds? Or it ends early because event polling is di It ends shortly since event processing is disabled. So either time is OK as long as it's not too short. Here is an example timeline: Query Compilation: 697.760ms - Continuing without syncing Metastore events: 45.467ms (45.467ms) - Metadata load started: 45.817ms (350.628us) - Metadata load finished. loaded-tables=1/1 load-requests=1 catalog-updates=7 storage-load-time=87ms: 453.859ms (408.041ms) - Analysis finished: 572.427ms (118.567ms) - Authorization finished (noop): 572.959ms (532.457us) - Value transfer graph computed: 588.568ms (15.609ms) - Single node plan created: 610.693ms (22.124ms) - Runtime filters computed: 616.271ms (5.578ms) - Distributed plan created: 617.244ms (972.512us) - Planning finished: 697.760ms (80.516ms) Changed it to 10s to avoid confusion. Timeout won't happen so the test takes the same amount of time. -- 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: 38 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: Thu, 20 Feb 2025 13:35:53 +0000 Gerrit-HasComments: Yes
