Saurabh Katiyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/21426 )
Change subject: IMPALA-12216: Print timestamp for impala-shell errors ...................................................................... Patch Set 16: (5 comments) > Patch Set 15: > > Build Successful > > https://jenkins.impala.io/job/gerrit-code-review-checks/16615/ : Initial code > review checks passed. Use gerrit-verify-dryrun-external or > gerrit-verify-dryrun to run full precommit tests. In continuation with last review, below changes were made : 1. Added TS_LEN variable to make code more readable 2. Regarding test "assert "[Cancelled]" not in result.stderr", have added it back and modified error message for cancellation on "ctrl+c" event >From : +++++++++++++++++++ 2024-08-05 00:12:15 [Cancelled] +++++++++++++++++++ To : +++++++++++++++++++ 2024-08-05 00:12:15 [Warning] Query Interrupted +++++++++++++++++++ > assert "[Cancelled]" not in result.stderr was added in > https://gerrit.cloudera.org/c/1529/ and decied to leave this test intact to > avoid any regression. Also, while investigating error in test failures, difference in behavior of impala-shell is identified with "ctrl+c" when connected over different protocol. This could be replicated in current GA impala-shell, I have opened a new bug jira to track this behavior: https://issues.apache.org/jira/browse/IMPALA-13266 http://gerrit.cloudera.org:8080/#/c/21426/14/tests/custom_cluster/test_hs2_fault_injection.py File tests/custom_cluster/test_hs2_fault_injection.py: http://gerrit.cloudera.org:8080/#/c/21426/14/tests/custom_cluster/test_hs2_fault_injection.py@184 PS14, Line 184: ab > To make the code more readable, can we make this a constant variable? Is it Yes, It's date time stamp prefix that i have removed before asserting the value to void flaky behavior of test: ++++++++++++++++++++++++ 2024-07-15 12:49:27 [Exception] type=<class 'socket.error'> in FetchResults. [Errno 4] Interrupted system call 2024-07-15 12:49:27 [Warning] Cancelling Query 2024-07-15 12:49:27 [Warning] close session RPC failed: <class 'shell_exceptions.QueryCancelledByShellException'> ++++++++++++++++++++++++ [20:] will ignore first 20 characters for 2024-07-15 12:49:27 part before asserting with expected string http://gerrit.cloudera.org:8080/#/c/21426/15/tests/custom_cluster/test_hs2_fault_injection.py File tests/custom_cluster/test_hs2_fault_injection.py: http://gerrit.cloudera.org:8080/#/c/21426/15/tests/custom_cluster/test_hs2_fault_injection.py@30 PS15, Line 30: # timestamp length that needs to be trimmed before asserting in multiple tests > flake8: E265 block comment should start with '# ' Done http://gerrit.cloudera.org:8080/#/c/21426/15/tests/custom_cluster/test_hs2_fault_injection.py@31 PS15, Line 31: > flake8: E225 missing whitespace around operator Done http://gerrit.cloudera.org:8080/#/c/21426/15/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/21426/15/tests/shell/test_shell_commandline.py@54 PS15, Line 54: > flake8: E225 missing whitespace around operator Done http://gerrit.cloudera.org:8080/#/c/21426/12/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/21426/12/tests/shell/test_shell_interactive.py@340 PS12, Line 340: assert impalad.wait_for_num_in_flight_queries(0) > How does the new format impact this? We now explicitly added keyword keyword [Cancelled] with timestamp (initial ask of this improvement jira) and the test was used to check no cancel keyword should be there in CLI output ++++++++++++++++++++++++ Scan Progress:[ ] 0% Query Progress:[ ] 0% ^C Cancelling Query Opened TCP connection to localhost:21050 2024-07-15 12:51:43 [Cancelled] [localhost:21050] default> ++++++++++++++++++++++++ Removing this has no impact on test as we are checking if "^C" character is there which means there was interrupt and we are asserting with number of in-flight queries -- To view, visit http://gerrit.cloudera.org:8080/21426 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4abbd02aa9f61210b0333495bf191e72c22a5944 Gerrit-Change-Number: 21426 Gerrit-PatchSet: 16 Gerrit-Owner: Saurabh Katiyal <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Saurabh Katiyal <[email protected]> Gerrit-Comment-Date: Sun, 04 Aug 2024 18:57:15 +0000 Gerrit-HasComments: Yes
