Repository: incubator-impala Updated Branches: refs/heads/master 6a9df5409 -> 8de2884e8
IMPALA-4880: Clarify synchronization policy for 'done_' in KuduScanNode This patch adds a comment to clarify that it's safe to optimistically read 'done_' in some places in KuduScanNode. This is to avoid some confusion as the code synchronizes access to the variable in certain places but not in others. Also, did some miscellaneous cleanup in a couple of places. Change-Id: I72c5f1fecf4bcf737d71a1fa13e59361604485f0 Reviewed-on: http://gerrit.cloudera.org:8080/5494 Reviewed-by: Matthew Jacobs <[email protected]> Tested-by: Impala Public 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/36711759 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/36711759 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/36711759 Branch: refs/heads/master Commit: 367117594ab7e7be0176601cf74962e875afa766 Parents: 6a9df54 Author: Sailesh Mukil <[email protected]> Authored: Tue Dec 13 16:14:57 2016 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Tue Feb 7 00:07:17 2017 +0000 ---------------------------------------------------------------------- be/src/exec/kudu-scan-node.cc | 22 +++++++++------------- be/src/exec/kudu-scan-node.h | 5 ++++- 2 files changed, 13 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/36711759/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 40689ee..630f5b7 100644 --- a/be/src/exec/kudu-scan-node.cc +++ b/be/src/exec/kudu-scan-node.cc @@ -183,12 +183,8 @@ Status KuduScanNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos *eos = true; } - Status status; - { - unique_lock<mutex> l(lock_); - status = status_; - } - return status; + unique_lock<mutex> l(lock_); + return status_; } void KuduScanNode::Close(RuntimeState* state) { @@ -219,7 +215,7 @@ void KuduScanNode::DebugString(int indentation_level, stringstream* out) const { const string* KuduScanNode::GetNextScanToken() { unique_lock<mutex> lock(lock_); - if (next_scan_token_idx_ >= scan_tokens_.size()) return NULL; + if (done_ || next_scan_token_idx_ >= scan_tokens_.size()) return nullptr; const string* token = &scan_tokens_[next_scan_token_idx_++]; return token; } @@ -254,7 +250,7 @@ void KuduScanNode::ThreadAvailableCb(ThreadResourceMgr::ResourcePool* pool) { Status KuduScanNode::ProcessScanToken(KuduScanner* scanner, const string& scan_token) { RETURN_IF_ERROR(scanner->OpenNextScanToken(scan_token)); bool eos = false; - while (!eos) { + while (!eos && !done_) { gscoped_ptr<RowBatch> row_batch(new RowBatch( row_desc(), runtime_state_->batch_size(), mem_tracker())); RETURN_IF_ERROR(scanner->GetNext(row_batch.get(), &eos)); @@ -282,6 +278,8 @@ void KuduScanNode::RunScannerThread(const string& name, const string* initial_to const string* scan_token = initial_token; Status status = scanner.Open(); if (status.ok()) { + // Here, even though a read of 'done_' may conflict with a write to it, + // ProcessScanToken() will return early, as will GetNextScanToken(). while (!done_ && scan_token != NULL) { status = ProcessScanToken(&scanner, *scan_token); if (!status.ok()) break; @@ -304,11 +302,9 @@ void KuduScanNode::RunScannerThread(const string& name, const string* initial_to { unique_lock<mutex> l(lock_); - if (!status.ok()) { - if (status_.ok()) { - status_ = status; - done_ = true; - } + if (!status.ok() && status_.ok()) { + status_ = status; + done_ = true; } // Decrement num_active_scanners_ unless handling the case of an early exit when // optional threads have been exceeded, in which case it already was decremented. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/36711759/be/src/exec/kudu-scan-node.h ---------------------------------------------------------------------- diff --git a/be/src/exec/kudu-scan-node.h b/be/src/exec/kudu-scan-node.h index 84e2531..3b896c8 100644 --- a/be/src/exec/kudu-scan-node.h +++ b/be/src/exec/kudu-scan-node.h @@ -93,7 +93,10 @@ class KuduScanNode : public ScanNode { /// Set to true when the scan is complete (either because all scan tokens have been /// processed, the limit was reached or some error occurred). - /// Protected by lock_ + /// Protected by lock_. It is safe to do optimistic reads without taking lock_ in + /// certain places, based on the decisions taken after that. + /// The tradeoff is occasionally doing some extra work versus increasing lock + /// contention. volatile bool done_; /// Thread group for all scanner worker threads
