IMPALA-3953: Fixes for KuduScanNode BE test failure After a previous fix for IMPALA-3857, KuduScanNodeTest TestLimitsAreEnforced (BE test) occasionally throws when a scanner thread takes a lock_ that isn't valid, crashing the process.
It looks like the issue is likely that TestScanEmptyString isn't closing its KuduScanNode, and a lingering ScannerThread may end up touching invalid memory later. This fixes the test case and also: 1) Adds some missing synchronization in KuduScanNode which was found in the process of investigating this issue. 2) Adds a DCHECK on ~KuduScanNode() to ensure it was closed. This was tested by running KuduScanNodeTest in a loop for 5 hours. Without the fix, the failure was produced within several hours. Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97 Reviewed-on: http://gerrit.cloudera.org:8080/3888 Reviewed-by: Tim Armstrong <[email protected]> Reviewed-by: Dan Hecht <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/6fc399eb Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/6fc399eb Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/6fc399eb Branch: refs/heads/master Commit: 6fc399ebc435121cdb7865ff4987aca1c95af5fc Parents: f4da925 Author: Matthew Jacobs <[email protected]> Authored: Wed Aug 10 13:35:52 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Mon Aug 15 23:41:08 2016 +0000 ---------------------------------------------------------------------- be/src/exec/kudu-scan-node-test.cc | 1 + be/src/exec/kudu-scan-node.cc | 3 +++ 2 files changed, 4 insertions(+) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/6fc399eb/be/src/exec/kudu-scan-node-test.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/kudu-scan-node-test.cc b/be/src/exec/kudu-scan-node-test.cc index 7719a75..0cda0ec 100644 --- a/be/src/exec/kudu-scan-node-test.cc +++ b/be/src/exec/kudu-scan-node-test.cc @@ -499,6 +499,7 @@ TEST_F(KuduScanNodeTest, TestScanEmptyString) { ASSERT_OK(scanner.GetNext(runtime_state_.get(), NULL, &eos)); ASSERT_TRUE(eos); ASSERT_EQ(PrintBatch(batch), "[(10 null )]\n"); + scanner.Close(runtime_state_.get()); } // Test that scan limits are enforced. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/6fc399eb/be/src/exec/kudu-scan-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/kudu-scan-node.cc b/be/src/exec/kudu-scan-node.cc index 74011b1..a7a068c 100644 --- a/be/src/exec/kudu-scan-node.cc +++ b/be/src/exec/kudu-scan-node.cc @@ -82,6 +82,7 @@ KuduScanNode::KuduScanNode(ObjectPool* pool, const TPlanNode& tnode, } KuduScanNode::~KuduScanNode() { + DCHECK(is_closed()); STLDeleteElements(&kudu_predicates_); } @@ -184,6 +185,8 @@ Status KuduScanNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos num_rows_returned_ -= num_rows_over; COUNTER_SET(rows_returned_counter_, num_rows_returned_); *eos = true; + + unique_lock<mutex> l(lock_); done_ = true; materialized_row_batches_->Shutdown(); }
