Huaisi Xu has posted comments on this change. Change subject: IMPALA-3385: Fix crashes on accessing error_log ......................................................................
Patch Set 11: (19 comments) http://gerrit.cloudera.org:8080/#/c/2829/11//COMMIT_MSG Commit Message: Line 18: hdfsScanNode::splitprocess > HdfsScanNode::ProcessSplit() Done Line 23: when returning error_log > and return a copy of the RuntimeState::error_log_, fixing a race in the coo Done http://gerrit.cloudera.org:8080/#/c/2829/11/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 1204: ss << endl; > is this endl still needed? i think you get one at the end of all logs, it w we need this since this might follow "... bytes." or "...)." http://gerrit.cloudera.org:8080/#/c/2829/11/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 221: // same errors multiple times. > This comment should be moved to the header comment for ClearErrorMap(). Al Done http://gerrit.cloudera.org:8080/#/c/2829/11/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 120: void GetErrors(ErrorLogMap* errors); > Since this is no longer a simple accessor, add a function comment and move Done Line 278: unreported_error_idx_ > delete Done http://gerrit.cloudera.org:8080/#/c/2829/11/be/src/util/error-util-test.cc File be/src/util/error-util-test.cc: Line 49: cleared[TErrorCode::RPC_TIMEOUT].messages.clear(); > What is this code trying to verify? Shouldn't it use ClearErrorMap() rather Done Line 70: right > kinda weird to be using 'right' on the left hand side. Consider just creati Done Line 86: ASSERT_EQ(0, cleared[TErrorCode::RPC_TIMEOUT].messages.size()); > order these cases consistently with lines 72-86. That makes it read much fa Done http://gerrit.cloudera.org:8080/#/c/2829/11/be/src/util/error-util.cc File be/src/util/error-util.cc: Line 136: LogEntry > log_entry Done Line 142: { > missing space Done Line 171: DCHECK_EQ(source.messages.empty()^(source.count == 0), 0); > DCHECK_EQ(source.messages.empty(), source.count == 0); Done Line 202: return errors.size(); > one line (and braces are required for multi-line if) Done http://gerrit.cloudera.org:8080/#/c/2829/11/tests/query_test/test_errorlog.py File tests/query_test/test_errorlog.py: Line 24: > add a brief summary of what this test wants to accomplish, and reference th Done Line 29: class TestErrorLogs(ImpalaTestSuite): > what happens when you run this test without your fixes? does it reproduce no.. I got a crash in mergeerror because I removed reporting thread's sleeping, so a cleared error log could get in before uncleared one arrives coordinator.. if ((*left).count(v.first) > 0) { (*left)[v.first].count += v.second.count; } else { (*left)[v.first].messages.push_back(v.second.messages.front()); (*left)[v.first].count = v.second.count; } and vector::front() crashes. Given this you think we need this test any more? Line 40: if os.getenv("TARGET_FILESYSTEM") in ["s3", "isilon", "local"]: return > could we just remove hbase dimension when running on these systems, or is t Done Line 49: location = vector.get_value('location') > Add comment: # Node ID 0 is the scan node. Done Line 67: cancel_result = self.client.cancel(handle) > why do we do this? is it just because otherwise the query runs for a long t Yes. that runs for a long time. Line 69: assert cancel_result.status_code == 0,\ > what gets returned if the query had completed before the cancel rpc? will yes, that rpc is idempotent. -- 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: 11 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
