You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2021/07/13 20:15:12 UTC

[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all cooridnators

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


Change subject: IMPALA-8762: Track host level admission stats across all cooridnators
......................................................................

IMPALA-8762: Track host level admission stats across all cooridnators

This patch adds the ability to share the per-host stats for locally
admitted queries across all cordinators. This helps to get a more
consolidated view of the cluster for stats like slots_in_use and
mem_admitted when making local admission decisions.

Testing:
Added e2e py test

Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M common/thrift/StatestoreService.thrift
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
5 files changed, 242 insertions(+), 68 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
Gerrit-Change-Number: 17683
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>

[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-8762: Track host level admission stats across all coordinators
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/17683/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17683/1//COMMIT_MSG@7
PS1, Line 7: coordinators
> nit: coordinators
Done


http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:

http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h@440
PS1, Line 440:   // This maps a backends's id(host/port id) to its host level statistics which are used
> Nit: Its not a struct any more
Done


http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h@442
PS1, Line 442: kends.
> nit: what's the map key? host_id?
Done


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

http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@75
PS1, Line 75: // "<prefix><pool_name><delimiter><backend_id>". "!" is used because the backend id
> Nit: is it clearer to say it is of the form?
Done


http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@244
PS1, Line 244: // Parses the topic key to separate the prefix that helps recognize the kind of update
> Nit: separate
Done


http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@245
PS1, Line 245: // received.
> Nit: received
Done


http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@1652
PS1, Line 1652:       if (topic_backend_id == host_id_) continue;
> This is skipping updates about the current host?
this skips the update that was added by the current host in its previous statestore update, since the current host's local version is more up to date. Similar to what we do in L1635 above


http://gerrit.cloudera.org:8080/#/c/17683/1/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:

http://gerrit.cloudera.org:8080/#/c/17683/1/tests/custom_cluster/test_executor_groups.py@96
PS1, Line 96:     self.num_impalads = 2
> why is this 2?
this is a mis-nomer, it basically refers to the num of impalad instances that self._start_impala_cluster will look for. See L66 and L80.
I'll update the naming of the variables to represent their true meaning.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
Gerrit-Change-Number: 17683
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 20:50:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-8762: Track host level admission stats across all coordinators
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
Gerrit-Change-Number: 17683
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Jul 2021 23:23:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.

Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17683 )

Change subject: IMPALA-8762: Track host level admission stats across all coordinators
......................................................................


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
Gerrit-Change-Number: 17683
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 21:58:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-8762: Track host level admission stats across all coordinators
......................................................................


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/17683/2/be/src/scheduling/admission-controller.cc@257
PS2, Line 257:       << "All admission topic key prefixes should be of the same size";
> Nit: "All admission topic keys" is better, as this may make it clearer for 
Done


http://gerrit.cloudera.org:8080/#/c/17683/2/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:

http://gerrit.cloudera.org:8080/#/c/17683/2/tests/custom_cluster/test_executor_groups.py@573
PS2, Line 573:     # identical but this will at least test that code path as a sanity check.
> Nit: "at least" "code path"
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
Gerrit-Change-Number: 17683
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Jul 2021 23:22:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-8762: Track host level admission stats across all coordinators
......................................................................


Patch Set 3: Code-Review+2

Carrying over Andrew's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243
Gerrit-Change-Number: 17683
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Jul 2021 23:22:48 +0000
Gerrit-HasComments: No