Huaisi Xu has posted comments on this change. Change subject: IMPALA-3385: Fix crashes on accessing error_log ......................................................................
Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/2829/8/be/src/util/error-util.cc File be/src/util/error-util.cc: Line 141: if (v.second.count == 0) continue; > combine L140-141: } else if (v.second.count != 0) { Done Line 160: BOOST_FOREACH(const ErrorLogMap::value_type& v, right) { > it'd be easier to read this code if we did: Done Line 164: DCHECK_EQ((*left)[v.first].count, 0); > DCHECKs shouldn't have side-effects, but operator[] on map does. let's just Done Line 168: } else { > let's just do: But if an error has not been seen in left but cleared in right wont be merged if we do this. Line 170: DCHECK_EQ((*left)[v.first].messages.empty()^((*left)[v.first].count == 0), false); > these DCHECKs are way too hard to read. The first can just be: see above Line 171: if (!v.second.messages.empty()) { > if ((*left)[error].messages.empty()) { Done Line 185: DCHECK(!it->second.messages.empty()); > this is not right. the bug you are trying to fix is that the ErrorLogMap ca Sorry.. I followed one of you comment without thinking. Line 197: cit->second.messages.size() - !cit->second.messages.empty() : 0; > simpler written like: Sorry. this was not correct when general error message is only cleared. (we should -1 since we count all occurrence since last clear). it is better we can use count to track this the same way. what do you think? maybe it is easier to increase general error's count. -- To view, visit http://gerrit.cloudera.org:8080/2829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a7e3d22e26147ada780aae5aed1f2e25a515afc Gerrit-PatchSet: 8 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
