Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21587 )
Change subject: IMPALA-13115: Add query id to error messages ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/21587/8/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: http://gerrit.cloudera.org:8080/#/c/21587/8/be/src/service/impala-beeswax-server.cc@244 PS8, Line 244: RaiseBeeswaxException I don't see a callsite of it in this function. Can we mention RAISE_IF_ERROR() instead? Or "RaiseBeeswaxException() used in RAISE_IF_ERROR()". http://gerrit.cloudera.org:8080/#/c/21587/8/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/21587/8/be/src/service/impala-hs2-server.cc@173 PS8, Line 173: status->__set_errorMessage(exec_status.GetDetail()); Just checking all the callsites of __set_errorMessage(), are we missing the query id here and at L164, L184? http://gerrit.cloudera.org:8080/#/c/21587/8/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/21587/8/shell/impala_shell.py@709 PS8, Line 709: ('\nCancelled' in err_msg or '\nInvalid or unknown query handle' in err_msg): Can we add a comment for this change? http://gerrit.cloudera.org:8080/#/c/21587/8/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/21587/8/tests/beeswax/impala_beeswax.py@62 PS8, Line 62: return self.__message Is this required by this patch? http://gerrit.cloudera.org:8080/#/c/21587/8/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/21587/8/tests/common/test_result_verifier.py@827 PS8, Line 827: return actual_msg.find(expected_msg, m.end()) != -1 Can we use assert to have better error messages when this failed? With assert, we can add the 'actual_msg' at the end and it will be printed when the assert failed. -- To view, visit http://gerrit.cloudera.org:8080/21587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67e659681e36162cad1d9684189106f8eedbf092 Gerrit-Change-Number: 21587 Gerrit-PatchSet: 8 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Thu, 01 Aug 2024 09:11:37 +0000 Gerrit-HasComments: Yes
