Huaisi Xu has posted comments on this change. Change subject: IMPALA-3385: Fix crashes on accessing error_log ......................................................................
Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/2829/5/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 223: } > Returning a copy is kind of surprising. How about making the interface mirr done Line 242: // errors multiple times. > This comment doesn't make sense, as far as I see. What goes wrong if we ju I thought about this, but if we clear the whole map then errorcount is reset every 5 seconds by default. not sure if this is good. I can have a counter in runtime state to track some of them. This can make the code look good, i can just use std::move(). what do you think? http://gerrit.cloudera.org:8080/#/c/2829/5/be/src/util/error-util.cc File be/src/util/error-util.cc: Line 135: if (v.second.count == 0) continue; > please also test that you don't break ErrorLog functionality. For example, right.. this was a left over of my previous change, where I changed appenderror to make that more general. Line 140: } else { > DCHECK_GT(v.second.messages, 0); Done Line 142: if (v.second.count < 2) { > if (v.second.count == 1) { Done Line 159: if (v.second.count == 0) continue; > remove - this doesn't work for GENERAL. see above. Done Line 161: if (v.first == TErrorCode::GENERAL) { > DCHECK_EQ(v.second.count, 0); Done Line 167: (*left)[v.first].count += v.second.count; > this is still broken if 'left' was cleared. left will never be cleared. This wont be called on per fragment's runtime state. that uses LogError Line 168: } else { > DCHECK_GT(v.second.count, 0); Done Line 181: if (it != map->end()) { > DCHECK(!it->second.messages.empty()); Done Line 193: errors.find(TErrorCode::GENERAL)->second.messages.size() - 1 : 0; > this code looks wrong -- messages.size() can be 0, so general_errors will u Done. If we can avoid using ErrorCount than this wont be a problem. -- 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: 5 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
