You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Marcel Kornacker (Code Review)" <ge...@cloudera.org> on 2017/06/02 21:46:19 UTC
[Impala-ASF-CR] IMPALA-5384: Simplify coordinator locking protocol
Marcel Kornacker has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7065
Change subject: IMPALA-5384: Simplify coordinator locking protocol
......................................................................
IMPALA-5384: Simplify coordinator locking protocol
This changes the locking behavior of the coordinator in
the following way:
- the central lock_ is replaced with a state variable
protected by a lock; the state distinguishes between
execution and (multiple) error states and is used
to maintain the overall query status
- data structures related to stats about Insert operations
are moved into a new class InsertExecState
Also:
- adds DescriptorTbl::CreateHdfsTableDescriptor to avoid
having to create an entire DescriptorTbl during INSERT
finalization (when only a descriptor for the output
table is needed)
- removes TQueryExecRequest.desc_tbl, there's already
a home for it in TQueryContext.desc_tbl
Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/plan-root-sink.cc
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/fragment-instance-state.cc
A be/src/runtime/insert-exec-state.cc
A be/src/runtime/insert-exec-state.h
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/counting-barrier.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
33 files changed, 1,140 insertions(+), 935 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/7065/1
--
To view, visit http://gerrit.cloudera.org:8080/7065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-5384: Simplify coordinator locking protocol
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-5384: Simplify coordinator locking protocol
......................................................................
Patch Set 2:
This passes the pre-commit tests.
--
To view, visit http://gerrit.cloudera.org:8080/7065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5384: Simplify coordinator locking protocol
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has uploaded a new patch set (#2).
Change subject: IMPALA-5384: Simplify coordinator locking protocol
......................................................................
IMPALA-5384: Simplify coordinator locking protocol
This changes the locking behavior of the coordinator in
the following way:
- the central lock_ is replaced with a state variable
protected by a lock; the state distinguishes between
execution and (multiple) error states and is used
to maintain the overall query status
- data structures related to stats about Insert operations
are moved into a new class InsertExecState
Also:
- adds DescriptorTbl::CreateHdfsTableDescriptor to avoid
having to create an entire DescriptorTbl during INSERT
finalization (when only a descriptor for the output
table is needed)
- removes TQueryExecRequest.desc_tbl, there's already
a home for it in TQueryContext.desc_tbl
Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/plan-root-sink.cc
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/fragment-instance-state.cc
A be/src/runtime/insert-exec-state.cc
A be/src/runtime/insert-exec-state.h
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/in-process-servers.cc
M be/src/util/counting-barrier.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
33 files changed, 1,127 insertions(+), 932 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/7065/2
--
To view, visit http://gerrit.cloudera.org:8080/7065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-5384: Simplify coordinator locking protocol
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has abandoned this change. ( http://gerrit.cloudera.org:8080/7065 )
Change subject: IMPALA-5384: Simplify coordinator locking protocol
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/7065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
Gerrit-Change-Number: 7065
Gerrit-PatchSet: 2
Gerrit-Owner: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
[Impala-ASF-CR] IMPALA-5384: Simplify coordinator locking protocol
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7065 )
Change subject: IMPALA-5384: Simplify coordinator locking protocol
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7065/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/7065/2/be/src/runtime/coordinator.cc@487
PS2, Line 487: ReleaseResources();
Started looking at resurrecting the ExecState part of this patch (the rest has been committed in various commits). From reading through he code, I think there is a race between these two state transitions:
EXECUTING -> RETURNED_RESULTS
RETURNED_RESULTS -> ERROR_AFTER_RETURNED_RESULTS
The tail end of those transitions (after lock is dropped) can race so that there are concurrent executions of ReleaseResources(), ComputeQuerySummary(), etc.
We should be able to eliminate this race by eliminating the ERROR_AFTER_RETURNED_RESULTS state by having RETURNED_RESULTS also cause backend cancellation -- however, that will require untangling the whole ComputeQuerySummary mess to make sure we get full profiles for successful queries.
Alternatively, as a transitional step we might be able to remove that state without cancelling during RETURNED_RESULTS -- I think that's effectively how the code works today given the !returned_all_results_ check in UpdateBackendExecStatus() (that is arguable a bug, though).
In other words, make it so terminal states are never transitioned out of to another terminal state (which guarantees the terminal state handling code is executing exactly once).
--
To view, visit http://gerrit.cloudera.org:8080/7065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
Gerrit-Change-Number: 7065
Gerrit-PatchSet: 2
Gerrit-Owner: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Apr 2018 18:05:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5384: Simplify coordinator locking protocol
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7065 )
Change subject: IMPALA-5384: Simplify coordinator locking protocol
......................................................................
Patch Set 2:
This has been subsumed by another set of patches that were derived from this one.
--
To view, visit http://gerrit.cloudera.org:8080/7065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a79aa38e529d0994921906b2beb796fd589a7e3
Gerrit-Change-Number: 7065
Gerrit-PatchSet: 2
Gerrit-Owner: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
Gerrit-Comment-Date: Tue, 15 May 2018 17:40:37 +0000
Gerrit-HasComments: No