Riza Suminto 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 32: (7 comments) http://gerrit.cloudera.org:8080/#/c/20131/31//COMMIT_MSG Commit Message: 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 > Yes. The improvement mentioned below can also help this. Ack 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@1549 PS31, Line 1549: > Yeah. Usually timeoutMs is larger than 1s and hmsEventSyncSleepIntervalMs_ Done 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); > We want to show this error message to the client as well. E.g. I think existing query_handle->GetAnalysisWarnings() should be the preferred method here. You can: 1. make waitForHmsEvents return String. 2. If not an empty String, make getTExecRequest() append that String to an ArrayList<String> (for future-proofing, in case we want to piggy-back other warnings from other place). 3. Let getTExecRequest() pass the ArrayList down to doCreateExecRequest() where we have analyzer for the first time. 4. Iterate the ArrayList and append the items to analysisResult.getAnalyzer().addWarning(). I understand that TExecRequest.analysis_warnings in Frontend.thrift might have different context than TQueryCtx.warnings in Query.thift. But this warning is raised by Frontend, so I think it is appropriate to add it to TExecRequest.analysis_warnings. 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@1378 PS31, Line 1378: onfirms if the table exists. The describe table command will fail if the table > The value of sync_hms_events_wait_time_s comes from the parameter so we can Done 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@144 PS31, Line 144: impala_client.close() : > Done. It seems a burden to create and close the impala_client for each meth If you are referring to default impala clients inside ImpalaTestSuite, they do initialized at setup_class() and closed at teardown_class(). And I agree with you, that ideally the client should reset to its default query options at every setup_method() / teardown_method() to guard against one test method disturbing other test method in single py.test class. http://gerrit.cloudera.org:8080/#/c/20131/32/tests/metadata/test_event_processing_base.py File tests/metadata/test_event_processing_base.py: http://gerrit.cloudera.org:8080/#/c/20131/32/tests/metadata/test_event_processing_base.py@37 PS32, Line 37: impala_client = cls.create_impala_client() : try: nit: You can use with-as instead of try-catch with cls.create_impala_client() as impala_client: ImpalaConnection is compatible with python Context Manager. https://github.com/apache/impala/blob/c936f95af2b3efef685aee095e4ecfd550472f3c/tests/common/impala_connection.py#L89-L90 http://gerrit.cloudera.org:8080/#/c/20131/32/tests/metadata/test_event_processing_base.py@60 PS32, Line 60: e > flake8: E501 line too long (91 > 90 characters) nit: Please address this. -- 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: 32 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 18:32:19 +0000 Gerrit-HasComments: Yes
