Tim Armstrong has posted comments on this change. Change subject: IMPALA-5198: test_exchange_delays custom cluster test failed on s3 tests ......................................................................
Patch Set 1: (3 comments) This is a nice catch http://gerrit.cloudera.org:8080/#/c/6627/1//COMMIT_MSG Commit Message: Line 7: IMPALA-5198: test_exchange_delays custom cluster test failed on s3 tests Maybe describe the symptoms/bug more generally, so it's easier to understand the impact. E.g. "Error message sometimes doesn't is dropped in HS2". http://gerrit.cloudera.org:8080/#/c/6627/1/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: Line 288: if (exec_state->coord() != NULL) { Is this addition related to the change? I don't understand why we're printing progress to the error log. http://gerrit.cloudera.org:8080/#/c/6627/1/be/src/util/error-util.cc File be/src/util/error-util.cc: Line 123: if (detail.size() > 0) { It seems like conceptually the problem is the Status(TStatus) constructor should do the inverse of Status::ToThrift() but doesn't actually. This fix solves the problem but now the packing of msg and details into a vector happens at a different level to the unpacking. I think this code would be easier to understand if it was restructured slightly so that the packing and unpacking happened at the same level. Maybe we should just change ErrorMsg() to store a vector internally? Or we could add a method to ErrorMsg() to return a vector with all of the messages, which would be the inverse of the constructor? -- To view, visit http://gerrit.cloudera.org:8080/6627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
