[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-14 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 9:

This change did not cherrypick successfully into branch 2.x. To resolve this, 
please do the cherry-pick manually and submit it to Gerrit at refs/for/2.x or 
add an exception to the branch 2.x copy of bin/ignored_commits.json. Thanks, 
your friendly bot at https://jenkins.impala.io/job/cherrypick-2.x-and-test/491/ 
.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 14 May 2018 22:10:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 8: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 12 May 2018 01:43:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..

IMPALA-6957: calc thread resource requirement in planner

This only factors in fragment execution threads. E.g. this does *not*
try to account for the number of threads on the old Thrift RPC
code path if that is enabled.

This is loosely related to the old VCores estimate, but is different in
that it:
* Directly ties into the notion of required threads in
  ThreadResourceMgr.
* Is a strict upper bound on the number of such threads, rather than
  an estimate.

Does not include "optional" threads. ThreadResourceMgr in the backend
bounds the number of "optional" threads per impalad, so the number of
execution threads on a backend is limited by

  sum(required threads per query) +
  CpuInfo::num_cores() * FLAGS_num_threads_per_core

DCHECKS in the backend enforce that the calculation is correct. They
were actually hit in KuduScanNode because of some races in thread
management leading to multiple "required" threads running. Now the
first thread in the multithreaded scans never exits, which means
that it's always safe for any of the other threads to exit early,
which simplifies the logic a lot.

Testing:
Updated planner tests.

Ran core tests.

Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Reviewed-on: http://gerrit.cloudera.org:8080/10256
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins 
---
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/thread-resource-mgr.h
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M common/thrift/Frontend.thrift
M common/thrift/ImpalaInternalService.thrift
M common/thrift/Planner.thrift
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/ResourceProfile.java
M fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
43 files changed, 1,977 insertions(+), 1,931 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10256/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test:

http://gerrit.cloudera.org:8080/#/c/10256/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@64
PS4, Line 64: instances
> Sounds like deferring it is a good idea :)
Agree, definitely outside of the scope of this change



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 11 May 2018 23:16:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2456/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 11 May 2018 22:29:05 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 11 May 2018 22:28:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 11 May 2018 21:01:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Frontend.thrift@416
PS6, Line 416: max_per_host_required_threads
> should we just go ahead and call this a reservation?  Or, we can wait for l
Done. Went through and updated variables and comments to use 
thread_reservation/mem_reservation.

There were some minor formatting changes from clang-format too.


http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Planner.thrift
File common/thrift/Planner.thrift:

http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Planner.thrift@80
PS6, Line 80: required_threads
> same question about reservation.
Done


http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java:

http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@276
PS6, Line 276: .setMemEstimateBytes(0)
> why do that?
The ResourceProfileBuilder requires setting a mem estimate (to avoid PlanNodes 
implicitly leaving it at zero). This is just preserving the old behaviour 
(which doesn't make sense, but I've been deferring making piecemeal changes to 
the estimates - IMPALA-5013).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 11 May 2018 19:58:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Tim Armstrong (Code Review) 
Hello Bikramjeet Vig, Alex Behm, Dan Hecht,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#7).

Change subject: IMPALA-6957: calc thread resource requirement in planner
..

IMPALA-6957: calc thread resource requirement in planner

This only factors in fragment execution threads. E.g. this does *not*
try to account for the number of threads on the old Thrift RPC
code path if that is enabled.

This is loosely related to the old VCores estimate, but is different in
that it:
* Directly ties into the notion of required threads in
  ThreadResourceMgr.
* Is a strict upper bound on the number of such threads, rather than
  an estimate.

Does not include "optional" threads. ThreadResourceMgr in the backend
bounds the number of "optional" threads per impalad, so the number of
execution threads on a backend is limited by

  sum(required threads per query) +
  CpuInfo::num_cores() * FLAGS_num_threads_per_core

DCHECKS in the backend enforce that the calculation is correct. They
were actually hit in KuduScanNode because of some races in thread
management leading to multiple "required" threads running. Now the
first thread in the multithreaded scans never exits, which means
that it's always safe for any of the other threads to exit early,
which simplifies the logic a lot.

Testing:
Updated planner tests.

Ran core tests.

Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
---
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/thread-resource-mgr.h
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M common/thrift/Frontend.thrift
M common/thrift/ImpalaInternalService.thrift
M common/thrift/Planner.thrift
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/ResourceProfile.java
M fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
43 files changed, 1,977 insertions(+), 1,931 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/10256/7
--
To view, visit http://gerrit.cloudera.org:8080/10256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-11 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Frontend.thrift@416
PS6, Line 416: max_per_host_required_threads
should we just go ahead and call this a reservation?  Or, we can wait for later 
if/when we actually put some backend reservation accounting/limit in place. 
(Though we can still think of it as a reservation of an unlimited resource 
currently). Just thinking it would be nice standardize on resource terminology 
- this is effectively the min-thread-reservation. (and then we should clarify 
the thing above is a mem reservation).

Okay to defer or argue against of course.


http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Planner.thrift
File common/thrift/Planner.thrift:

http://gerrit.cloudera.org:8080/#/c/10256/6/common/thrift/Planner.thrift@80
PS6, Line 80: required_threads
same question about reservation.


http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java:

http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@276
PS6, Line 276: .setMemEstimateBytes(0)
why do that?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 11 May 2018 17:09:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-10 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
File fe/src/main/java/org/apache/impala/planner/PlanFragment.java:

http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@261
PS6, Line 261: .setRequiredThreads(1).build()
> Why is this 1 and not getNumInstancesPerHost()
The resource profile being computed here is for each instance of the fragment 
(see the method comment), computing the per-host value is done outside of this.


http://gerrit.cloudera.org:8080/#/c/10256/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test:

http://gerrit.cloudera.org:8080/#/c/10256/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@64
PS4, Line 64: instances
> I like short names. I don't think a longer name will necessarily make it cl
Sounds like deferring it is a good idea :)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 10 May 2018 23:51:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-10 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10256 )

Change subject: IMPALA-6957: calc thread resource requirement in planner
..


Patch Set 6:

(2 comments)

FE and explain changes look good to me

http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
File fe/src/main/java/org/apache/impala/planner/PlanFragment.java:

http://gerrit.cloudera.org:8080/#/c/10256/6/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@261
PS6, Line 261: .setRequiredThreads(1).build()
Why is this 1 and not getNumInstancesPerHost()


http://gerrit.cloudera.org:8080/#/c/10256/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test:

http://gerrit.cloudera.org:8080/#/c/10256/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@64
PS4, Line 64: instances
> I'm open to the idea but maybe we should decouple it from this change?
I like short names. I don't think a longer name will necessarily make it 
clearer. Even if we call it "total-instances-estimate" I can definitely see 
users asking about its precise meaning.

Many things here are estimates, so not sure it makes sense to add an explicit 
"estimate" prefix.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 10 May 2018 23:42:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6957: calc thread resource requirement in planner

2018-05-08 Thread Tim Armstrong (Code Review) 
Hello Bikramjeet Vig, Dan Hecht,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#6).

Change subject: IMPALA-6957: calc thread resource requirement in planner
..

IMPALA-6957: calc thread resource requirement in planner

This only factors in fragment execution threads. E.g. this does *not*
try to account for the number of threads on the old Thrift RPC
code path if that is enabled.

This is loosely related to the old VCores estimate, but is different in
that it:
* Directly ties into the notion of required threads in
  ThreadResourceMgr.
* Is a strict upper bound on the number of such threads, rather than
  an estimate.

Does not include "optional" threads. ThreadResourceMgr in the backend
bounds the number of "optional" threads per impalad, so the number of
execution threads on a backend is limited by

  sum(required threads per query) +
  CpuInfo::num_cores() * FLAGS_num_threads_per_core

DCHECKS in the backend enforce that the calculation is correct. They
were actually hit in KuduScanNode because of some races in thread
management leading to multiple "required" threads running. Now the
first thread in the multithreaded scans never exits, which means
that it's always safe for any of the other threads to exit early,
which simplifies the logic a lot.

Testing:
Updated planner tests.

Ran core tests.

Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
---
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/thread-resource-mgr.h
M common/thrift/Frontend.thrift
M common/thrift/Planner.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/ResourceProfile.java
M fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
33 files changed, 1,837 insertions(+), 1,797 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/10256/6
--
To view, visit http://gerrit.cloudera.org:8080/10256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I982837ef883457fa4d2adc3bdbdc727353469140
Gerrit-Change-Number: 10256
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong