Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21587 )
Change subject: IMPALA-13115: Add query id to error messages ...................................................................... Patch Set 9: (5 comments) > Patch Set 8: > > (5 comments) Thanks for reviewing! 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: RAISE_IF_ERROR(). > I don't see a callsite of it in this function. Can we mention RAISE_IF_ERRO Thanks! Changed to 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_statusCode(thrift::TStatusCode::ERROR_STATUS); > Just checking all the callsites of __set_errorMessage(), are we missing the Thanks! Added at all the three places. 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: if err_msg in ['ERROR: Cancelled', 'ERROR: Invalid or unknown query handle'] or \ > Can we add a comment for this change? Thanks! Added. 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? Yes. Otherwise `error_msg_expected()` will always return `False` since the error message does not start with "Query <query_id> failed", but starts with "ImpalaBeeswaxException". 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 asse Thanks! I tested locally and found that if there is an error, pytest will display the values of the arguments of `error_msg_expected()` , including `actual_msg`. -- 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: 9 Gerrit-Owner: Xuebin Su <x...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Xuebin Su <x...@cloudera.com> Gerrit-Comment-Date: Fri, 02 Aug 2024 07:08:36 +0000 Gerrit-HasComments: Yes
