Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8100 )
Change subject: IMPALA-5940. Avoid log spew by using Status::Expected. ...................................................................... Patch Set 1: (1 comment) Thanks for the review! I added in the two messages that Mostafa mentioned on the JIRA. Doing so introduced two complexities: * Whereas I believe we could never log the messages in the first draft, the new messages being silenced here (especially RPC timeout) should be logged. * Now that there's no stack trace, the fact that the messages come as being from "status.cc" is not useful. I see the following ways forward: * Use draft 1 to silence the easy messages; ignore this new draft 2. * Use draft 2. Notably, it uses the __FILE__ preprocessor macro. __func__ is also a possibility. * Change the usage pattern. For example STATUS_AND_LOG(...) could be a macro that does VLOG and the Status generation. I was very hesitant to add macros for this so didn't go that route. * Something else? I want to highlight a few more things about patch 2: * I started off with a signature like Status(..., bool silent, bool omit_backtrace), but there were only 3 possible good states. I had bugs in my implementation, so decided on an enum instead. It's all private, so there's little API risk here. * There's a copy of ErrorMsg going on when Status::NoTrace() takes an error code and a string. We do several of them in various Status signatures. Given that we used to also compute an entire back trace, I think this is ok. * I didn't add a new test. I didn't find any tests that look like they're testing our logging behavior. Happy for suggestions, though I think it's overkill. * I don't know how to reproduce RPC timeouts. I "kill -STOP"ed an impalad, but that wasn't enough. Curious for pointers. http://gerrit.cloudera.org:8080/#/c/8100/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8100/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-5940. Avoid log spew by using Status::Expected. > IMPALA-5940: (colon not dot) Done -- To view, visit http://gerrit.cloudera.org:8080/8100 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330 Gerrit-Change-Number: 8100 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Tue, 26 Sep 2017 23:03:30 +0000 Gerrit-HasComments: Yes
