Dan Hecht 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) { Line 160: BOOST_FOREACH(const ErrorLogMap::value_type& v, right) { it'd be easier to read this code if we did: TErrorCode::type error = v.first; and then use that in place of v.first everywhere. Line 164: DCHECK_EQ((*left)[v.first].count, 0); DCHECKs shouldn't have side-effects, but operator[] on map does. let's just delete this second dcheck -- this part of the code is just assuming that the right count is 0 (and so doesn't need to do the count +=, so the first dcheck is sufficient). Line 168: } else { let's just do: } else if (v.second.count != 0) { and then see comment at L171 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: DCHECK(!v.second.messages.empty()); This is valid with the else-if added to line 168. just delete the second for the same reason above (side-effects, and the code here doesn't leverage that invariant anyway). Line 171: if (!v.second.messages.empty()) { if ((*left)[error].messages.empty()) { it's more direct and avoids a bunch of unnecessary allocation/frees. Line 185: DCHECK(!it->second.messages.empty()); this is not right. the bug you are trying to fix is that the ErrorLogMap can have an entry with count == 0 and messages.empty(): so this DCHECK should fire in your testing. How are you testing this change? Please get your repros/tests working before sending another change for review. Line 197: cit->second.messages.size() - !cit->second.messages.empty() : 0; simpler written like: cit != errors.end() && !cit->second.messages.empty() ? cit->second.messges.size() - 1 : 0; -- 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
