[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..

IMPALA-3134: Support different proc mem limits among impalads for
admission control checks

Currently the admission controller assumes that all backends have the
same process mem limit as the impalad it itself is running on. With
this patch the proc mem limit for each impalad is available to the
admission controller and it uses it for making correct admisssion
decisions. It currently works under the assumption that the
per-process memory limit does not change dynamically.

Testing:
Added an e2e test.

IMPALA-5662: Log the queuing reason for a query

The queuing reason is now logged both while queuing for the first
time and while trying to dequeue.

Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Reviewed-on: http://gerrit.cloudera.org:8080/10396
Reviewed-by: Bikramjeet Vig 
Tested-by: Impala Public Jenkins 
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M bin/start-impala-cluster.py
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_admission_controller.py
9 files changed, 127 insertions(+), 38 deletions(-)

Approvals:
  Bikramjeet Vig: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 22 May 2018 22:27:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 22 May 2018 19:03:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 5: Code-Review+2

Carrying Tim's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 22 May 2018 19:02:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..

IMPALA-3134: Support different proc mem limits among impalads for
admission control checks

Currently the admission controller assumes that all backends have the
same process mem limit as the impalad it itself is running on. With
this patch the proc mem limit for each impalad is available to the
admission controller and it uses it for making correct admisssion
decisions. It currently works under the assumption that the
per-process memory limit does not change dynamically.

Testing:
Added an e2e test.

IMPALA-5662: Log the queuing reason for a query

The queuing reason is now logged both while queuing for the first
time and while trying to dequeue.

Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M bin/start-impala-cluster.py
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_admission_controller.py
9 files changed, 127 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/10396/5
--
To view, visit http://gerrit.cloudera.org:8080/10396
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10396/4/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

http://gerrit.cloudera.org:8080/#/c/10396/4/common/thrift/StatestoreService.thrift@75
PS4, Line 75:   // The process memory limit of this backend.
> "in bytes"?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 22 May 2018 19:01:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10396/4/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

http://gerrit.cloudera.org:8080/#/c/10396/4/common/thrift/StatestoreService.thrift@75
PS4, Line 75:   // The process memory limit of this backend.
"in bytes"?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 22 May 2018 18:50:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..

IMPALA-3134: Support different proc mem limits among impalads for
admission control checks

Currently the admission controller assumes that all backends have the
same process mem limit as the impalad it itself is running on. With
this patch the proc mem limit for each impalad is available to the
admission controller and it uses it for making correct admisssion
decisions. It currently works under the assumption that the
per-process memory limit does not change dynamically.

Testing:
Added an e2e test.

IMPALA-5662: Log the queuing reason for a query

The queuing reason is now logged both while queuing for the first
time and while trying to dequeue.

Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M bin/start-impala-cluster.py
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_admission_controller.py
9 files changed, 127 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/10396/4
--
To view, visit http://gerrit.cloudera.org:8080/10396
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10396/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/10396/3/be/src/scheduling/admission-controller.cc@408
PS3, Line 408:  =
 :   make_pair(nullptr, std::numeric_limits::max())
I think you can just initialise this directly:


  pair min_proc_mem_limit(nullptr, 
std::numeric_limits::max());



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 18 May 2018 23:54:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10396/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/10396/3/be/src/scheduling/admission-controller.cc@408
PS3, Line 408: pair min_proc_mem_limit =
 :   make_pair(nullptr, std::numeric_limits::max());
> @Tim: let me know if you decide not to adopt this pattern for your patch fo
WFM


http://gerrit.cloudera.org:8080/#/c/10396/3/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:

http://gerrit.cloudera.org:8080/#/c/10396/3/tests/custom_cluster/test_admission_controller.py@454
PS3, Line 454: exec_options['mem_limit'] = "3G"
Does this line do anything? It's already set to 3G above.
Is it intentional that num_nodes=1 still at this point?

It might be easier to follow if we just created a copy of exec_options for each 
query rather than mutating the one version. We use the copy() function in a few 
other tests for this purpose. What do you think?


http://gerrit.cloudera.org:8080/#/c/10396/3/tests/custom_cluster/test_admission_controller.py@473
PS3, Line 473:"2.00 GB but only 1.00 GB out of 2.00 GB 
was available.", str(e)), str(e)
long line



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 18 May 2018 19:20:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10396/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/10396/3/be/src/scheduling/admission-controller.cc@408
PS3, Line 408: pair min_proc_mem_limit =
 :   make_pair(nullptr, std::numeric_limits::max());
@Tim: let me know if you decide not to adopt this pattern for your patch for 
IMPALA-6035



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 17 May 2018 21:24:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..

IMPALA-3134: Support different proc mem limits among impalads for
admission control checks

Currently the admission controller assumes that all backends have the
same process mem limit as the impalad it itself is running on. With
this patch the proc mem limit for each impalad is available to the
admission controller and it uses it for making correct admisssion
decisions. It currently works under the assumption that the
per-process memory limit does not change dynamically.

Testing:
Added an e2e test.

Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M bin/start-impala-cluster.py
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_admission_controller.py
8 files changed, 119 insertions(+), 32 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 2:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/10396/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10396/2//COMMIT_MSG@14
PS2, Line 14: decisions.
> Can you mention that this assumes that the per-process memory limit does no
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@49
PS2, Line 49: int64_t GetProcMemLimit() {
> This is dead code now
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@152
PS2, Line 152: const string HOST_MEM_NOT_AVAILABLE = "Not enough memory 
available on host $0."
> Maybe this error message should include the total process memory limit, e.g
good idea. Done.


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@450
PS2, Line 450:
> Unnecessary vertical whitespace
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@458
PS2, Line 458:
> Vertical whitespace
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/query-schedule.h@69
PS2, Line 69:   // The process memory limit of this backend.
> Maybe mention that it's the value obtained from the statestore during sched
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/10396/2/bin/start-impala-cluster.py@87
PS2, Line 87: seperated
> separated
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:

http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@429
PS2, Line 429: 1000
> Can we set this lower? Is there a reason that it needs to sit in the queue
forgot to update that.


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@431
PS2, Line 431:   def test_heterogeneous_proc_mem_limit(self, vector):
> Can we add a 3GB query that succeeds? E.g. with num_nodes=1, submitted to o
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@433
PS2, Line 433: mem limits of each impalad"""
> Maybe add an extra sentence to explain the concept of the test. e.g. "Start
Done


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@434
PS2, Line 434: # Choose a query that runs on all 3 backends.
> Are all these queries submitted to the first impalad? It's not clear if tha
modified a test to send a query to a different impalad. (check the queuing test)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 17 May 2018 21:18:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10396/2/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/10396/2/bin/start-impala-cluster.py@87
PS2, Line 87: seperated
separated



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 15 May 2018 17:50:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10396 )

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..


Patch Set 2:

(10 comments)

Looks good, had some minor comments.

http://gerrit.cloudera.org:8080/#/c/10396/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10396/2//COMMIT_MSG@14
PS2, Line 14: decisions.
Can you mention that this assumes that the per-process memory limit does not 
change dynamically? Might not be obvious.


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@49
PS2, Line 49: int64_t GetProcMemLimit() {
This is dead code now


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@152
PS2, Line 152: const string HOST_MEM_NOT_AVAILABLE = "Not enough memory 
available on host $0."
Maybe this error message should include the total process memory limit, e.g. 
"$2/$3 was available"


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@450
PS2, Line 450:
Unnecessary vertical whitespace


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@458
PS2, Line 458:
Vertical whitespace


http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/query-schedule.h@69
PS2, Line 69:   // The process memory limit of this backend.
Maybe mention that it's the value obtained from the statestore during 
scheduling. We don't expect it to change right now, but that may be important 
context if we ever want to change to dynamic process memory limits.


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:

http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@429
PS2, Line 429: 1000
Can we set this lower? Is there a reason that it needs to sit in the queue for 
1s?


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@431
PS2, Line 431:   def test_heterogeneous_proc_mem_limit(self, vector):
Can we add a 3GB query that succeeds? E.g. with num_nodes=1, submitted to one 
of the 3GB nodes.


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@433
PS2, Line 433: mem limits of each impalad"""
Maybe add an extra sentence to explain the concept of the test. e.g. "Starts a 
cluster where the first impalad has a smaller proc mem limit than other 
impalads and run queries where admission/reject depends on the coordinator 
knowing the other impalad's mem limits".


http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@434
PS2, Line 434: # Choose a query that runs on all 3 backends.
Are all these queries submitted to the first impalad? It's not clear if that's 
the intent.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 15 May 2018 17:44:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..

IMPALA-3134: Support different proc mem limits among impalads for
admission control checks

Currently the admission controller assumes that all backends have the
same process mem limit as the impalad it itself is running on. With
this patch the proc mem limit for each impalad is available to the
admission controller and it uses it for making correct admisssion
decisions.

Testing:
Added an e2e test.

Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M bin/start-impala-cluster.py
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_admission_controller.py
8 files changed, 95 insertions(+), 24 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3134: Support different proc mem limits among impalads for admission control checks

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10396


Change subject: IMPALA-3134: Support different proc mem limits among impalads 
for admission control checks
..

IMPALA-3134: Support different proc mem limits among impalads for
admission control checks

Currently the admission controller assumes that all backends have the
same process mem limit as the impalad it itself is running on. With
this patch the proc mem limit for each impalad is available to the
admission controller and it uses it for making correct admisssion
decisions.

Testing:
Added an e2e test.

Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M bin/start-impala-cluster.py
M common/thrift/StatestoreService.thrift
M tests/custom_cluster/test_admission_controller.py
8 files changed, 95 insertions(+), 24 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e
Gerrit-Change-Number: 10396
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig