[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 3: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


IMPALA-4180: Synchronize accesses to RuntimeState::reader_contexts_

HdfsScanNodeBase::Close() may add its outstanding DiskIO context to
RuntimeState::reader_contexts_ to be unregistered later when the
fragment is closed. In a plan fragment with multiple HDFS scan nodes,
it's possible for HdfsScanNodeBase::Close() to be called concurrently.
To allow safe concurrent accesses, this change adds a SpinLock to
synchronize accesses to 'reader_contexts_' in RuntimeState.

Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Reviewed-on: http://gerrit.cloudera.org:8080/4558
Reviewed-by: Dan Hecht 
Tested-by: Internal Jenkins
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test
5 files changed, 39 insertions(+), 7 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Dan Hecht: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 3:

Taras also verified the fix works with the same query.

-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 3:

> (1 comment)

Taras, were you also able to verify that the fix solves the crash?

-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4558/3/testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test
File testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test:

Line 166: select * from t t1 left join t t2 on t1.x > 0
> did you verify that this query reproduces a crash?
Confirmed by Taras. Thanks Taras !


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4558/3/testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test
File testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test:

Line 166: select * from t t1 left join t t2 on t1.x > 0
did you verify that this query reproduces a crash?


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#3).

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..

IMPALA-4180: Synchronize accesses to RuntimeState::reader_contexts_

HdfsScanNodeBase::Close() may add its outstanding DiskIO context to
RuntimeState::reader_contexts_ to be unregistered later when the
fragment is closed. In a plan fragment with multiple HDFS scan nodes,
it's possible for HdfsScanNodeBase::Close() to be called concurrently.
To allow safe concurrent accesses, this change adds a SpinLock to
synchronize accesses to 'reader_contexts_' in RuntimeState.

Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test
5 files changed, 39 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4558/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 302:   for (DiskIoRequestContext* context: reader_contexts_) {
> Nit clang-format puts a space before :, should do that or consistency.
Done


http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 165:   /// Takes ownership of a scan node's reader context and unregisters 
it when the
> "Takes ownership of the given reader context which may still hold used IO b
It's "automatic" from the perspective of the caller but I know what you mean.


Line 167:   /// TODO: Attach the reader context to the last row batch instead.
> Works for me. If the eventual direction is not clear, then let's remove the
Sure. Let's remove it for now but that sounds like an attractive option if 
unregistration happens at the end of the lifetime of the IO buffer.


http://gerrit.cloudera.org:8080/#/c/4558/2/testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test
File testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test:

Line 215:  QUERY
> move this below the test for IMPALA-561 to cluster related tests together
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 167:   /// TODO: Attach the reader context to the last row batch instead.
> This TODO seems too speculative to me, unclear whether the proposed approac
Works for me. If the eventual direction is not clear, then let's remove the 
TODO.


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 302:   for (DiskIoRequestContext* context: reader_contexts_) {
Nit clang-format puts a space before :, should do that or consistency.


http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 167:   /// TODO: Attach the reader context to the last row batch instead.
This TODO seems too speculative to me, unclear whether the proposed approach is 
the right one, since it's usually simpler not to attach things to row batches, 
and the current approach seems fine to me.


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4558/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 304:   }
> reader_contexts_.clear()
Done


http://gerrit.cloudera.org:8080/#/c/4558/1/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 167:   void DeferIOContextUnregistration(DiskIoRequestContext* 
reader_context);
> AcquireReaderContext()?
Done


Line 170:   void UnregisterIOContexts();
> UnregisterReaderContexts()?
Done


http://gerrit.cloudera.org:8080/#/c/4558/1/testdata/workloads/functional-query/queries/QueryTest/single-node-topn.test
File 
testdata/workloads/functional-query/queries/QueryTest/single-node-topn.test:

Line 4: # whose children are scan nodes. When scan nodes are closed 
concurrently, make
> I don't think the last sentence is necessary. It's likely to become stale p
Rephrased the comment in the new patch.


http://gerrit.cloudera.org:8080/#/c/4558/1/tests/query_test/test_queries.py
File tests/query_test/test_queries.py:

Line 131:   def test_single_node_topn(self, vector):
> This new test seems kind of misleading. The cause of this are two scan node
The new test is moved to single-node-nlj.test in the new patch. Rewrote the 
query to be NLJ which also exercises the same path.


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#2).

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..

IMPALA-4180: Synchronize accesses to RuntimeState::reader_contexts_

HdfsScanNodeBase::Close() may add its outstanding DiskIO context to
RuntimeState::reader_contexts_ to be unregistered later when the
fragment is closed. In a plan fragment with multiple HDFS scan nodes,
it's possible for HdfsScanNodeBase::Close() to be called concurrently.
To allow safe concurrent accesses, this change adds a SpinLock to
synchronize accesses to 'reader_contexts_' in RuntimeState.

Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M testdata/workloads/functional-query/queries/QueryTest/single-node-nlj.test
5 files changed, 38 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4558/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 

[Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts

[Michael Ho (Code Review)]

Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4558

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
..

IMPALA-4180: Synchronize accesses to RuntimeState::reader_contexts_

HdfsScanNodeBase::Close() may add its outstanding DiskIO context to
RuntimeState::reader_contexts_ to be unregistered later when the
fragment is closed. In a plan fragment with multiple HDFS scan nodes,
it's possible for HdfsScanNodeBase::Close() to be called concurrently.
To allow safe concurrent accesses, this change adds a SpinLock to
synchronize accesses to 'reader_contexts_' in RuntimeState.

Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
A testdata/workloads/functional-query/queries/QueryTest/single-node-topn.test
M tests/query_test/test_queries.py
6 files changed, 46 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4558/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho