Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22577 )
Change subject: IMPALA-13822: Add more detail in impala_connection.py logs ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py@178 PS1, Line 178: e, value, Fals > Nit: "must not be empty". Removed this. http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py@310 PS1, Line 310: s > Is it intentional that we removed the comment mark ("--") from the beginnin Yes, this is intentional as the '--' is already incorporated in LOG_FORMAT. http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py@402 PS1, Line 402: > maybe pass log_lines to log_client() and move the if block there? this woul Changed to do the logging at set_configuration() They currently look like this: -- 2025-03-05 07:33:26,743 INFO MainThread: beeswax: set client_identifier=query_test/test_exprs.py::TestExprLimits::()::test_expr_child_limit[protocol:beeswax|table_format:text/none|exec_option:{'test_replan':1;'batch_size':0;'num_nodes':0;'disable_codegen_rows_threshold':0;'disable_codegen':False;'abort_on_error':1;'exec_singl; -- 2025-03-05 07:33:26,743 INFO MainThread: beeswax: set_configuration: set test_replan=1; set batch_size=0; set num_nodes=0; set disable_codegen_rows_threshold=0; set disable_codegen=False; set abort_on_error=1; set exec_single_node_rows_threshold=0; http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py@408 PS1, Line 408: b > "if log_lines" seems clearer to me, as this checks for None mainly Removed. http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py@411 PS1, Line 411: n\n > isn't one \n enough? The double \n is to space out sql from log line right above it. It follows the current convention. http://gerrit.cloudera.org:8080/#/c/22577/1/tests/common/impala_connection.py@684 PS1, Line 684: {5} > We could add some description before the SQL, something like I don't think it adds value. Currently, it already look long like this: -- 2025-03-05 07:40:02,186 INFO MainThread: hs2: executing against Impala at localhost:21050. session: 1c42ae0d26536de6:550232d5471bc693 main_cursor: True user: None DROP DATABASE IF EXISTS `test_kudu_insert_e88f2c67` CASCADE; -- To view, visit http://gerrit.cloudera.org:8080/22577 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44ea7fbec15684ac5379703f781a400b4f17da8d Gerrit-Change-Number: 22577 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 05 Mar 2025 15:47:22 +0000 Gerrit-HasComments: Yes
