impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4180: Synchronize accesses to RuntimeState::reader contexts
Date Thu, 29 Sep 2016 04:30:31 GMT
Alex Behm 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()


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()?

I think the detail that these can only be reader contexts is important. "IOContext" seems
more general, e.g. read/write contexts are not interesting here.

The "Acquire" terminology is more similar to what we use elsewhere. We are transferring ownership
of the context to the runtime state because the context lives until the end of the fragment.

Might be good to add a brief TODO to indicate what the eventual solution should look like,
e.g., attach the resources to the final row batch.


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


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 pretty soon (at
least I hope so).


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 nodes within the same
fragment, the top-n nodes just happen to trigger the concurrent Close() pattern. This bug
seems more in line with IMPALA-561 where you added a test to single-node-nlj.test.


-- 
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 <kwho@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message