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 7: (4 comments) > Patch Set 5: > > (4 comments) Thanks for reviewing! http://gerrit.cloudera.org:8080/#/c/21587/5/be/src/service/child-query.cc File be/src/service/child-query.cc: http://gerrit.cloudera.org:8080/#/c/21587/5/be/src/service/child-query.cc@81 PS5, Line 81: TGetResultSetMetadataReq meta_req; > Do you have a scenario where it isn't? Thank you! After digging into the code, I found that errors in this function is returned as a Status object, rather than reported to the client. So I reverted this part of the change. http://gerrit.cloudera.org:8080/#/c/21587/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21587/5/be/src/service/impala-server.cc@a1258 PS5, Line 1258: > If every caller is now required to set this up, please add a note about it Thank you! It is not required by the caller of `Execute()` to set this up, but by the caller of the error reporting function, like `RaiseBeeswaxException()` and `HS2_RETURN_ERROR()`. So I added comments there. http://gerrit.cloudera.org:8080/#/c/21587/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/21587/5/shell/impala_shell.py@708 PS5, Line 708: if err_msg in ['ERROR: Cancelled', 'ERROR: Invalid or unknown query handle'] or \ > impala-shell is often used from a pypi release, so it won't necessarily mat Thank you! Indeed compatibility is very important. I added back the old pattern matching here. Moreover, for the "query id not found" check below, I changed it back to the old pattern matching but match only the error detail so that it can work with both old and new server versions, and both Beeswax and HS2 protocols. http://gerrit.cloudera.org:8080/#/c/21587/5/tests/custom_cluster/test_sys_db.py File tests/custom_cluster/test_sys_db.py: http://gerrit.cloudera.org:8080/#/c/21587/5/tests/custom_cluster/test_sys_db.py@64 PS5, Line 64: "table '{0}' failed to create but for the wrong reason:\n{1}\n" \ > we should also print the error e in the assert message Thank you! Added error e to the assertion message. -- 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: 7 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: Fri, 26 Jul 2024 09:10:33 +0000 Gerrit-HasComments: Yes
