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 2019/08/19 20:26:44 UTC
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14103
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
IMPALA-8858: Add observability of idle executor groups
This patch adds a metric that displays a comma seperated list of
executor group names that are in a healthy state and are idle
according to the coordinator (no queries admitted locally by the
coordinator are running on them). It gets updated when either the
cluster membership changes or a query gets admitted or released by the
admission controller.
Testing:
Added a custom cluster test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
6 files changed, 151 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/1
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/4/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/4/tests/custom_cluster/test_executor_groups.py@387
PS4, Line 387: =
> flake8: E711 comparison to None should be 'if cond is None:'
Done
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Aug 2019 00:15:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/11/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/11/be/src/scheduling/admission-controller.h@a834
PS11, Line 834:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
rebased and cleaned up some code around these methods
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 11
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 21:53:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 20: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 20
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Sep 2019 02:43:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 18
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 20:10:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#15).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 367 insertions(+), 212 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/15
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 15
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 16: Code-Review+2
(2 comments)
LGTM, a few typos that could be ignored
http://gerrit.cloudera.org:8080/#/c/14103/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14103/16//COMMIT_MSG@10
PS16, Line 10: will have a metric added that display the number of queries
Nit: displays
http://gerrit.cloudera.org:8080/#/c/14103/16/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/16/tests/custom_cluster/test_executor_groups.py@354
PS16, Line 354: # Create and exec group of min size 2 to exercise the case where a group becomes
Nit: Create an exec group...
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 16
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 18:35:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#2).
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
IMPALA-8858: Add observability of idle executor groups
This patch adds a metric that displays a comma seperated list of
executor group names that are in a healthy state and are idle
according to the coordinator (no queries admitted locally by the
coordinator are running on them). It gets updated when either the
cluster membership changes or a query gets admitted or released by the
admission controller.
Testing:
Added a custom cluster test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
6 files changed, 151 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/2
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 17:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14103/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14103/16//COMMIT_MSG@10
PS16, Line 10: will have a metric added that displays the number of queries
> Nit: displays
Done
http://gerrit.cloudera.org:8080/#/c/14103/16/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/16/tests/custom_cluster/test_executor_groups.py@354
PS16, Line 354: # Create an exec group of min size 2 to exercise the case where a group becomes
> Nit: Create an exec group...
Done
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 17
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 20:09:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#19).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that displays the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
- Added a custom cluster test and a BE metric test.
- Had to modify some metric tests that relied on ordering of metrics by
their name.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 392 insertions(+), 228 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/19
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 19
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#9).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
12 files changed, 316 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/9
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 9
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@364
PS8, Line 364:
: TUpdateExecutorMembershipRequest update_req;
: for (const auto& it : snapshot->current_backends) {
: const TBackendDescriptor& backend = it.second;
: if (backend.is_executor) {
: update_req.hostnames.insert(backend.address.hostname);
: update_req.ip_addresses.insert(backend.ip_address);
: update_req.num_executors++;
: }
: }
: Status status = this->frontend()->UpdateExecutorMembership(update_req);
: if (!status.ok()) {
: LOG(WARNING) << "Error updating frontend membership snapshot: "
: << status.GetDetail();
: }
> I'm inclined to think that this code should not be in the ExecEnv (but we c
As discussed offline, moving this out into a local helper function.
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@494
PS8, Line 494: std::
> nit: remove std:: in .cc files
I think some header file is adding all of the boost namespace because of which this unordered_set defaults to using the boost version instead of std
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/admission-controller.h@948
PS8, Line 948: load metric of the 'grp_name' executor group by 'delta'
> While reading this I was confused what 'delta' is, maybe instead of "query
Done. I feel 'delta' would work better with UpdateExecGroupMetricLocked since increment has a connotation of always increasing. Dont feel too strongly so let me know if you still prefer IncExecGroupMetricLocked over this.
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.h@124
PS8, Line 124: typedef std::function<void(SnapshotPtr)> UpdateCallbackFn;
> Previously, UpdateLocalServerFn was only called when backends had been dele
yup, I highlighted this in my previous comment (https://gerrit.cloudera.org/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@342)
The reason I did not add that bool was so that I can make this callback fn completely generic and leave it upto the clients/consumers to decide how to interpret the update and was banking on cluster membership changes being infrequent enough.
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@321
PS8, Line 321: SetStateAndUpdateMetrics(new_state);
> I know I asked for the metric update to be moved there, but now that we gen
I thought of the same but wanted to keep callbacks for external dependencies. Moreover, BlacklistExecutor() method requires us to not send out updates to callbacks, but it would still need to update metrics
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@322
PS8, Line 322:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@323
PS8, Line 323: NotifyAllCallbackUpdateFns
> I think NotifyListeners(new_state) might be more concise but clear enough
Done
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json@2502
PS6, Line 2502: ad.$0"
> maybe even num-running is ok (see my comment elsewhere about "load"). How d
switched to to the same as impala-server.backend-num-queries-executing
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 8
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 00:40:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 16:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4579/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 16
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:59:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 4:
@all Please let me know if the changes to enable metric deletion make sense.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Aug 2019 00:14:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#11).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 345 insertions(+), 167 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/11
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 11
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 12:
fixed clang-tidy error
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 12
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 22:39:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4357/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Aug 2019 01:03:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14103/15/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/15/tests/custom_cluster/test_executor_groups.py@90
PS15, Line 90: =
flake8: E712 comparison to True should be 'if cond is True:' or 'if cond:'
http://gerrit.cloudera.org:8080/#/c/14103/15/tests/custom_cluster/test_executor_groups.py@100
PS15, Line 100: =
flake8: E712 comparison to True should be 'if cond is True:' or 'if cond:'
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 15
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:13:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 19: Code-Review+2
Reviewed ordering-in-test results changes
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 19
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 22:30:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 3:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/14103/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14103/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-8858: Add observability of idle executor groups
I think it would be good to also expose the total number of healthy executor groups per pool while you're at it. This would help to determine overall service readiness (e.g. some users may want to wait until at least one healthy group is available).
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/admission-controller.h@408
PS3, Line 408: seperated
nit: typo
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/admission-controller.cc@83
PS3, Line 83: "admission-controller.executor-groups.total-healthy-idle";
Do we actually need these per resource pool? In the future we might have multiple pools and we already have the association between groups and pools through the name-prefix.
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/admission-controller.cc@1852
PS3, Line 1852: vector<std::string> GetIdleExecutorGroupNames(
This should probably call GetExecutorGroupsForPool() (see previous comment)
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.h@136
PS3, Line 136: Metric
Remove the word "Metric", it's an implementation detail what the receiver of the callback does with it.
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.h@221
PS3, Line 221: Updating
nit lowercase
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@342
PS3, Line 342: UpdateFrontendExecutorMembership(*new_backend_map, *new_executor_groups);
I think it would be cleaner to call all callbacks from a single spot. That will also allow us to move them into a list and loop over them, if we add even more.
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@346
PS3, Line 346: UpdateMetrics(new_state);
Should we call UpdateMetrics() from SetState()?
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@551
PS3, Line 551: SnapshotPtr new_state){
: const BackendIdMap& current_backends = new_state->current_backends;
: const ExecutorGroups& executor_groups = new_state->executor_groups;
What was the reason for this change?
http://gerrit.cloudera.org:8080/#/c/14103/3/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/3/common/thrift/metrics.json@2485
PS3, Line 2485: seperated
nit: typo
http://gerrit.cloudera.org:8080/#/c/14103/3/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/3/tests/custom_cluster/test_executor_groups.py@341
PS3, Line 341: self._add_executor_group(group_name_suffixes[0], 1, max_concurrent_queries=1)
Add num_executors parameter to be explicit, maybe also spell out min_size= here and below?
I think it would be good to express the intent in a comment, too, in particular why you're creating them with different sizes.
http://gerrit.cloudera.org:8080/#/c/14103/3/tests/custom_cluster/test_executor_groups.py@346
PS3, Line 346: self.coordinator.service.get_metric_value(
: "admission-controller.executor-groups.total-healthy-idle").split(",")
A helper like self._get_idle_groups() might make this more readable.
http://gerrit.cloudera.org:8080/#/c/14103/3/tests/custom_cluster/test_executor_groups.py@362
PS3, Line 362: # Splitting on an empty string with a delimiter returns ['']
This quirk could then be hidden inside the helper, too
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Aug 2019 16:36:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#12).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 345 insertions(+), 168 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/12
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 12
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4316/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 23:28:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 19:
Had changed the internal data structures used to store metrics and child metric groups from map to unordered map, which caused some metric tests to fail that relied on those being ordered.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 19
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 21:00:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 20:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4972/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 20
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 22:31:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#3).
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
IMPALA-8858: Add observability of idle executor groups
This patch adds a metric that displays a comma seperated list of
executor group names that are in a healthy state and are idle
according to the coordinator (no queries admitted locally by the
coordinator are running on them). It gets updated when either the
cluster membership changes or a query gets admitted or released by the
admission controller.
Testing:
Added a custom cluster test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
6 files changed, 155 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/3
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 12:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/runtime/exec-env.cc@501
PS12, Line 501: [server](ClusterMembershipMgr::SnapshotPtr snapshot) {
nit: I'd consider following the FE example and add a helper for this, but since it's less code and inside a shorter function, I'm also OK with keeping it here.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h@508
PS12, Line 508: QueryAndMemory
I thought the shorter version was also good, especially since there's still only one parameter.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h@988
PS12, Line 988: /// Updates the list of executor groups for which we maintain the query load metrics.
nit: indentation
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h@995
PS12, Line 995: Should
nit: Isn't this mandatory, i.e. shouldn't we say "Must be called"?
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@425
PS12, Line 425: // Now update the pool stats.
Is there a reason why we do host stats and pool stats in different ordere here and below? It's all under the lock, right?
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@431
PS12, Line 431: PoolStats* pool_stats = GetPoolStats(schedule);
nit: Follow the same pattern above and here (temporary for pool_stats or not, add to pools_for_updates_ or not?
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@432
PS12, Line 432: pool_stats->AdmitQueryAndMemory(schedule);
no need to add to pools_for_updates_?
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@1955
PS12, Line 1955: Locked
The "Locked" part of the name confused me at some of the caller sites because other methods that are called together with this one don't have it in their name.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@1958
PS12, Line 1958: (
nit: single line?
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.h@65
PS12, Line 65: receive notifications
Mention that no notifications are sent out for blacklisted nodes (see longer comment in cc file)
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc@390
PS12, Line 390: // - The update sent to the impala server is used to cancel queries on backends that
: // are no longer present in 'current_backends', but we don't remove the executor from
: // 'current_backends' here, since it always reflects the full statestore membership.
: // This avoids cancelling queries that may still be running successfully, eg. if the
: // backend is still up but got blacklisted due to a flaky network. If the backend
: // really is down, we'll still cancel the queries when the statestore removes it from
: // the membership.
: // - The update sent to the admission controller is used to maintain a metric for each
: // group having at least 1 executor that keeps track of the queries running on it. We
: // can defer that to the statestore update and avoid unnecessary metric deletions due
: // to flaky backends being blacklisted.
: // - The update sent to frontend is used to notify the planner, but the executors the
: // planner schedules things on is just a hint and the Scheduler will still see the
: // updated membership and choose non-blacklisted executors regardless of what the
: // planner says, so its fine to wait until the next topic update to notify the fe.
I think we need to mention this in the header file near the top where we explain the callbacks. Now that the code is more generic, this part still assumes that only these three callbacks are registering. If someone adds callbacks in the future they might expect them to be called whenever the state changes. In addition we should explain that callers should not hold on to the snapshot pointer in the callbacks (I thought we could actually do that in the AdmissionController and just reuse what we got in the last callback, but that way we would miss the blacklisting changes).
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc@443
PS12, Line 443: void ClusterMembershipMgr::SetState(const SnapshotPtr& new_state) {
nit: How would this look if we added a parameter "bool update_metrics" with a default to "true"? I don't feel strongly about it, feel free to ignore.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/service/impala-server.cc@1918
PS12, Line 1918: std::unordered_set<TUniqueId>::const_iterator
nit: could use auto, might even fit into the for loop
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h@337
PS12, Line 337: the pointer
nit: any pointers
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h@367
PS12, Line 367: the
nit: any pointers
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h@368
PS12, Line 368: so callers should take care not to use those and to
: /// get rid of any references to it to avoid dangling pointer issues
I think this part is not needed, it's implied by the first half of the sentence
http://gerrit.cloudera.org:8080/#/c/14103/12/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/12/common/thrift/metrics.json@2505
PS12, Line 2505: seperated
separated, I think the description is also stale.
http://gerrit.cloudera.org:8080/#/c/14103/12/common/thrift/metrics.json@2509
PS12, Line 2509: Total number of executor groups
This label doesn't match the description (number vs list)
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py@335
PS12, Line 335: Test
nit: Tests
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py@384
PS12, Line 384: "cluster-membership.executor-groups.total-healthy", 1, timeout=30)
also check here that executor-groups.total is still 2?
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py@411
PS12, Line 411: "cluster-membership.executor-groups.total-healthy", 0, timeout=30)
Also add checks for executor-groups.total here?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 12
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Sep 2019 22:12:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 15: Code-Review+1
Rebased and carrying over Lars's +1
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 15
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:13:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 2:
(1 comment)
A thing I thought of last night
http://gerrit.cloudera.org:8080/#/c/14103/2/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/2/be/src/scheduling/cluster-membership-mgr.cc@606
PS2, Line 606: idle_groups_->SetValue(idle_groups_str);
I think this means that when there are no idle groups then this will be "" empty string?
I'm worried that in all the stages we have to go through,
impala->metrics_prometheus->prometheus->autoscaler
that somewhere we will lose a metric with value "".
Would it be horrible to have the value be "none" in this case?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Aug 2019 15:50:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4524/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 10
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 20:48:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 19:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4599/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 19
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 21:38:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 5:
(4 comments)
Only looked at the deletion logic
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@334
PS5, Line 334: The client
nit: reword, something like "users of this class", or just mention that deletion invalidates pointers to the deleted metrics (but no other pointers).
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@375
PS5, Line 375: Is a no-op for non-transient metrics.
nit: It logs an error though, so a user shouldn't just call this without being reasonably sure that they're deleting a transient metric.
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@456
PS5, Line 456: boost::scoped_ptr<ObjectPool> obj_pool_;
The separation between the pool and the map seems complicated. Should we consider switching to the map altogether? Then metrics could just have a flag, or we could remove the distinction entirely.
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@462
PS5, Line 462: shared_ptr
why not use a unique_ptr?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 5
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Aug 2019 23:24:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/4/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/4/tests/custom_cluster/test_executor_groups.py@387
PS4, Line 387: =
flake8: E711 comparison to None should be 'if cond is None:'
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Aug 2019 00:14:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#4).
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
IMPALA-8858: Add observability of the query load on executor groups
With this patch, all healthy executor groups will have a metric added
that display the number of queries (admitted by the local coordinator)
running on them. The metric for a group is added when a it is healthy
and removed when its either not healthy or no longer exists. It gets
updated when either the cluster membership changes or a query gets
admitted or released by the admission controller.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
8 files changed, 244 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/4
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#6).
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
IMPALA-8858: Add observability of the query load on executor groups
With this patch, all healthy executor groups will have a metric added
that display the number of queries (admitted by the local coordinator)
running on them. The metric for a group is added when a it is healthy
and removed when its either not healthy or no longer exists. It gets
updated when either the cluster membership changes or a query gets
admitted or released by the admission controller.
Also adds the ability to delete metrics from a metric group after
registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
9 files changed, 210 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/6
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 6
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h@43
PS1, Line 43: /// This struct stores per-host statistics which are used during admission, by the cluster
I understand why this is here but I think semantically it belongs in the admission controller file. Similarly, the concept of an executor group being "idle" should move there. Have you tried adding it there and using a callback to trigger an update when the cluster membership changes?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 16:57:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4577/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 14
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:52:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4513/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 9
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 01:16:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4325/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Aug 2019 21:05:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@334
PS5, Line 334: invalidat
> nit: reword, something like "users of this class", or just mention that del
Done
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@375
PS5, Line 375:
> nit: It logs an error though, so a user shouldn't just call this without be
As suggested, removed the concept of transient metrics
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@456
PS5, Line 456: MetricMap metric_map_;
> The separation between the pool and the map seems complicated. Should we co
Good idea. I quickly scanned through the metrics in kudu/util and that seems to use something similar instead of an objectPool.
http://gerrit.cloudera.org:8080/#/c/14103/5/be/src/util/metrics.h@462
PS5, Line 462: Produces a
> why not use a unique_ptr?
we need a <void/Base class> type smart pointer that supports type erasure so we can store multiple subclass Metric types in the same map.
This post explains it pretty well https://stackoverflow.com/a/39288979
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 6
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Aug 2019 01:40:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 15:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4578/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 15
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:53:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4358/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 5
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Aug 2019 00:59:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4427/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 6
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Aug 2019 02:19:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 18:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4959/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 18
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 20:10:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#7).
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
IMPALA-8858: Add observability of the query load on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors or no longer
exists. It gets updated when either the cluster membership
changes or a query gets admitted or released by the admission
controller. Also adds the ability to delete metrics from a metric
group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
12 files changed, 264 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/7
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 7
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/runtime/exec-env.cc@161
PS9, Line 161: t : sna
> nit: hand off. How about SendClusterMembershipToFrontend()?
Done
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/runtime/exec-env.cc@177
PS9, Line 177:
> move the helper into an anonymous namespace above the impala namespace
Done
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.h@951
PS9, Line 951: const string& g
> Pass by const ref, looks like the implementation doesn't need a copy
Done
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc@1871
PS9, Line 1871: // There might be lingering queries from when this group was active.
> Maybe this code could go into a helper, e.g. MaxQueriesOnExecutorGroupHosts
good point, however I'll keep it for now to avoid passing around ClusterMembershipMgr::* typedefs
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc@1873
PS9, Line 1873: auto new_grp_it = snapshot->executor_groups.find(new_grp);
> usually we name iterators 'it', but since this is the second one in this me
Done
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc@1877
PS9, Line 1877: const string& host = TNetworkAddressToString(be_desc.address);
> const string&
Done
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 10
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 20:07:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 13: Code-Review+1
(3 comments)
Only some nits, otherwise LGTM.
http://gerrit.cloudera.org:8080/#/c/14103/13/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/13/be/src/scheduling/cluster-membership-mgr.h@68
PS13, Line 68: BlacklistExecutor
nit: BlacklistExecutor()
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/service/impala-server.cc@1918
PS12, Line 1918: // Add failed backend locations to all querie
> Done
Much better :)
http://gerrit.cloudera.org:8080/#/c/14103/13/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/13/tests/custom_cluster/test_executor_groups.py@385
PS13, Line 385: assert self.coordinator.service.get_metric_value(
This pattern occurs quite often, maybe a helper self._get_num_executor_groups(only_healthy = False) would be nice?
assert self.coordinator.service.get_metric_value("cluster-membership.executor-groups.total-healthy")
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 13
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Sep 2019 23:25:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#10).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
12 files changed, 317 insertions(+), 132 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/10
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 10
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4550/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 13
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Sep 2019 01:43:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 18: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4959/
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 18
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 00:22:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#14).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 367 insertions(+), 212 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/14
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 14
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#17).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that displays the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 367 insertions(+), 212 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/17
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 17
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#8).
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
IMPALA-8858: Add observability of the query load on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors or no longer
exists. It gets updated when either the cluster membership
changes or a query gets admitted or released by the admission
controller. Also adds the ability to delete metrics from a metric
group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
12 files changed, 265 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/8
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 8
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#5).
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
IMPALA-8858: Add observability of the query load on executor groups
With this patch, all healthy executor groups will have a metric added
that display the number of queries (admitted by the local coordinator)
running on them. The metric for a group is added when a it is healthy
and removed when its either not healthy or no longer exists. It gets
updated when either the cluster membership changes or a query gets
admitted or released by the admission controller.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
8 files changed, 244 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/5
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 5
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc@443
PS12, Line 443: void ClusterMembershipMgr::SetState(const SnapshotPtr& new_state) {
> nit: How would this look if we added a parameter "bool update_metrics" with
Since now we are updating metrics through the callback mechanism in the general case, I fell adding this param would make it confusing by introducing an option to manually update it here. the case in BlacklistExecutor should be the exception not the norm.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 12
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Sep 2019 01:27:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.h@943
PS6, Line 943: /// Updates the list of executor groups that we maintain the query load metrics for.
"groups for which we maintain..."
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.h@944
PS6, Line 944: or are not healthy
Should we consider keeping groups with >0 executors around, even if they're not healthy? In theory such a group could even run a query if an executor is not involved in execution of the query or finishes early before failing.
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.cc@320
PS6, Line 320: [this](std::unordered_set<string> healthy_grp_names) {
Can the argument be a const&?
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.cc@1852
PS6, Line 1852: while (it != healthy_exec_group_query_load_map_.end()) {
I feel that this function's body could benefit from a comment explaining what it does.
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/cluster-membership-mgr.h@135
PS6, Line 135: /// 'update_membership_lock_' is held when calling this callback.
The comment should include what gets passed into the callback, or alternative a typedef could make it more clear (see above).
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@342
PS3, Line 342: UpdateFrontendExecutorMembership(*new_backend_map, *new_executor_groups);
> I think it would be cleaner to call all callbacks from a single spot. That
I'm still in favor of this.
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@346
PS3, Line 346: UpdateMetrics(new_state);
> Should we call UpdateMetrics() from SetState()?
?
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/util/metrics.h@455
PS6, Line 455: typedef std::map<std::string, std::shared_ptr<Metric>> MetricMap;
Is there a reason this map needs to be ordered? If so, can you add it to the comment?
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json@2495
PS6, Line 2495: "description": "Total number of queries running on $0 executor group",
I'd suggest moving the $0 to the back of the string to make it slightly easier to read (and consistent with the label), but it's personal preference and I don't feel strongly about it.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 6
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Aug 2019 20:54:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14103/14/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/14/tests/custom_cluster/test_executor_groups.py@90
PS14, Line 90: =
flake8: E712 comparison to True should be 'if cond is True:' or 'if cond:'
http://gerrit.cloudera.org:8080/#/c/14103/14/tests/custom_cluster/test_executor_groups.py@100
PS14, Line 100: =
flake8: E712 comparison to True should be 'if cond is True:' or 'if cond:'
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 14
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:11:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 1:
This looks like the right approach.
Would there be any value by having a test in cluster-membership-mgr-test.cc ?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Aug 2019 23:49:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h@43
PS1, Line 43: /// The ClusterMembershipMgr manages the local backend's membership in the cluster. It has
> I think there's a case for making it admission-controller.executor-groups.t
Done
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Aug 2019 20:48:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 7:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.h@943
PS6, Line 943: /// Updates the list of executor groups for which we maintain the query load metrics.
> "groups for which we maintain..."
Done
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.h@944
PS6, Line 944: from the metric gr
> Should we consider keeping groups with >0 executors around, even if they're
With how the current cancellation logic works, the query is still counted as running against a backend by the impala-server till it gets un-registered, which means that it would be cancelled regardless of whether it has completed its fragments on a particular backend.
However, I still agree with keeping non-zero groups around since they can still have an intermediate non zero value for query load which can prove useful.
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.cc@320
PS6, Line 320: dequeue_thread_->Join();
> Can the argument be a const&?
changed this mechanism, switched to sending a snapshotPtr instead.
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/admission-controller.cc@1852
PS6, Line 1852: for (const auto& group : snapshot->executor_groups) {
> I feel that this function's body could benefit from a comment explaining wh
Done
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/scheduling/cluster-membership-mgr.h@135
PS6, Line 135: /// events. They may be called at any time before or after calling Init() and are
> The comment should include what gets passed into the callback, or alternati
changed the update mechanism
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@342
PS3, Line 342: base_snapshot = current_membership_.get();
> I'm still in favor of this.
Done.
Note: this adds a little extra work to the critical path since now we cant optimize work for specific calls:
- NotifyLocalServerForDeletedBackend called every time and not just when update_local_server is true
- Admission controller needs to separately create a set a groups instead of leveraging the set previously created during metric updates.
These are pretty minor + cluster changes would be way less frequent in practice so shouldn't matter much.
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@346
PS3, Line 346: // copying the Snapshot if it isn't.
> ?
Done
http://gerrit.cloudera.org:8080/#/c/14103/3/be/src/scheduling/cluster-membership-mgr.cc@551
PS3, Line 551:
:
:
> What was the reason for this change?
just seemed consistent with other update calls to send the whole snapshotPtr
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14103/6/be/src/util/metrics.h@455
PS6, Line 455: typedef std::unordered_map<std::string, std::shared_ptr<Metric>> MetricMap;
> Is there a reason this map needs to be ordered? If so, can you add it to th
oops, didnt notice that a map was being used instead of an unordered_map. Not sure what the initial intention was but I dont see any reason why it would require an ordered map. changing it to unordered_map. Same goes for ChildGroupMap below.
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json@2495
PS6, Line 2495: "description": "Total number of queries running on executor group: $0",
> I'd suggest moving the $0 to the back of the string to make it slightly eas
makes sense to me. Moving it to the end.
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json@2502
PS6, Line 2502: ad.$0"
do you think num-running-queries is more appropriate here?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 7
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Sep 2019 21:48:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 12:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/runtime/exec-env.cc@501
PS12, Line 501: [server](ClusterMembershipMgr::SnapshotPtr snapshot) {
> nit: I'd consider following the FE example and add a helper for this, but s
yup the reasons you mentioned is why I avoided adding another helper for this
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h@508
PS12, Line 508: QueryAndMemory
> I thought the shorter version was also good, especially since there's still
I change it to remain consistent with the separation for Release* methods to signify that it does both.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h@988
PS12, Line 988: /// Updates the list of executor groups for which we maintain the query load metrics.
> nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.h@995
PS12, Line 995: Should
> nit: Isn't this mandatory, i.e. shouldn't we say "Must be called"?
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@425
PS12, Line 425: // Now update the pool stats.
> Is there a reason why we do host stats and pool stats in different ordere h
no reason, I'll change it to follow the same order to avoid confusion.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@431
PS12, Line 431: PoolStats* pool_stats = GetPoolStats(schedule);
> nit: Follow the same pattern above and here (temporary for pool_stats or no
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@432
PS12, Line 432: pool_stats->AdmitQueryAndMemory(schedule);
> no need to add to pools_for_updates_?
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@1955
PS12, Line 1955: Locked
> The "Locked" part of the name confused me at some of the caller sites becau
That got carried over from when one of my previous implementations where i had multiple versions on this. Removing it now.
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/admission-controller.cc@1958
PS12, Line 1958: (
> nit: single line?
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.h@65
PS12, Line 65: receive notifications
> Mention that no notifications are sent out for blacklisted nodes (see longe
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/scheduling/cluster-membership-mgr.cc@390
PS12, Line 390: // - The update sent to the impala server is used to cancel queries on backends that
: // are no longer present in 'current_backends', but we don't remove the executor from
: // 'current_backends' here, since it always reflects the full statestore membership.
: // This avoids cancelling queries that may still be running successfully, eg. if the
: // backend is still up but got blacklisted due to a flaky network. If the backend
: // really is down, we'll still cancel the queries when the statestore removes it from
: // the membership.
: // - The update sent to the admission controller is used to maintain a metric for each
: // group having at least 1 executor that keeps track of the queries running on it. We
: // can defer that to the statestore update and avoid unnecessary metric deletions due
: // to flaky backends being blacklisted.
: // - The update sent to frontend is used to notify the planner, but the executors the
: // planner schedules things on is just a hint and the Scheduler will still see the
: // updated membership and choose non-blacklisted executors regardless of what the
: // planner says, so its fine to wait until the next topic update to notify the fe.
> I think we need to mention this in the header file near the top where we ex
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/service/impala-server.cc@1918
PS12, Line 1918: std::unordered_set<TUniqueId>::const_iterator
> nit: could use auto, might even fit into the for loop
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h@337
PS12, Line 337: the pointer
> nit: any pointers
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h@367
PS12, Line 367: the
> nit: any pointers
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/be/src/util/metrics.h@368
PS12, Line 368: so callers should take care not to use those and to
: /// get rid of any references to it to avoid dangling pointer issues
> I think this part is not needed, it's implied by the first half of the sent
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/12/common/thrift/metrics.json@2505
PS12, Line 2505: seperated
> separated, I think the description is also stale.
stale metric, removing it.
http://gerrit.cloudera.org:8080/#/c/14103/12/common/thrift/metrics.json@2509
PS12, Line 2509: Total number of executor groups
> This label doesn't match the description (number vs list)
stale metric, removing it.
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py@335
PS12, Line 335: Test
> nit: Tests
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py@384
PS12, Line 384: "cluster-membership.executor-groups.total-healthy", 1, timeout=30)
> also check here that executor-groups.total is still 2?
Done
http://gerrit.cloudera.org:8080/#/c/14103/12/tests/custom_cluster/test_executor_groups.py@411
PS12, Line 411: "cluster-membership.executor-groups.total-healthy", 0, timeout=30)
> Also add checks for executor-groups.total here?
replace with executor-groups.total only since a zero value for that would mean a zero for total-healthy
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 12
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Sep 2019 01:22:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#16).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 367 insertions(+), 212 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/16
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 16
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4462/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 7
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Sep 2019 22:26:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h@43
PS1, Line 43: /// This struct stores per-host statistics which are used during admission, by the cluster
> I agree (sply about HostStats feeling out of place) but currently the metri
I think there's a case for making it admission-controller.executor-groups.total-healthy-idle since "idle" is also an AC concept.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 1
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 23:00:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 20: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 20
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 22:31:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 9:
(7 comments)
Overall looking good, just had a few smaller things left
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/runtime/exec-env.cc@161
PS9, Line 161: handoff
nit: hand off. How about SendClusterMembershipToFrontend()?
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/runtime/exec-env.cc@177
PS9, Line 177: }
move the helper into an anonymous namespace above the impala namespace
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.h@951
PS9, Line 951: string grp_name
Pass by const ref, looks like the implementation doesn't need a copy
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc@1871
PS9, Line 1871: // There might be lingering queries from when this group was active.
Maybe this code could go into a helper, e.g. MaxQueriesOnExecutorGroupHosts()? It's not too long though, so I'm also ok with keeping it here.
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc@1873
PS9, Line 1873: auto itr = snapshot->executor_groups.find(new_grp);
usually we name iterators 'it', but since this is the second one in this method, maybe name it new_grp_it?
http://gerrit.cloudera.org:8080/#/c/14103/9/be/src/scheduling/admission-controller.cc@1877
PS9, Line 1877: const string host = TNetworkAddressToString(be_desc.address);
const string&
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@321
PS8, Line 321: SetStateAndUpdateMetrics(new_state);
> I thought of the same but wanted to keep callbacks for external dependencie
Thanks for the explanation. I think it's ok to use the external dependency plumbing for internal purposes, similar to how we call public methods from private methods. It should also be ok to update metrics from the blacklisting code directly without calling NotifyListeners (the other way around would be tricky, notify w/o updating metrics). Feel free to ignore if you feel strongly about it or it gets in your way in other places.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 9
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 04:17:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4528/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 12
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 23:20:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4463/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 8
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Sep 2019 22:33:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4304/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Aug 2019 20:54:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 16: Code-Review+1
(2 comments)
carrying over Lars's +1
http://gerrit.cloudera.org:8080/#/c/14103/15/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/15/tests/custom_cluster/test_executor_groups.py@90
PS15, Line 90:
> flake8: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Done
http://gerrit.cloudera.org:8080/#/c/14103/15/tests/custom_cluster/test_executor_groups.py@100
PS15, Line 100:
> flake8: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Done
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 16
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:19:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 17:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4591/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 17
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 20:50:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add observability of idle executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of idle executor groups
......................................................................
Patch Set 2:
(1 comment)
> This looks like the right approach.
> Would there be any value by having a test in cluster-membership-mgr-test.cc
> ?
I thought about that, but since the metric update method relies on both admissionController and the clusterMembershipManager, I felt an e2e test would suit it better. The e2e test covers all cases so it should provide enough coverage.
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/1/be/src/scheduling/cluster-membership-mgr.h@43
PS1, Line 43: /// This struct stores per-host statistics which are used during admission, by the cluster
> I understand why this is here but I think semantically it belongs in the ad
I agree (sply about HostStats feeling out of place) but currently the metrics in admission controller are all connected to resource pools (admission-controller.*.<pool_name>). This metric (cluster-membership.executor-groups.total-healthy-idle) which is tied to backends and executors groups fitted better inside the cluster membership manager.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 22:51:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that displays the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
- Added a custom cluster test and a BE metric test.
- Had to modify some metric tests that relied on ordering of metrics by
their name.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Reviewed-on: http://gerrit.cloudera.org:8080/14103
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 392 insertions(+), 228 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 21
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add observability of the query load on executor groups
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups
......................................................................
Patch Set 8:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@364
PS8, Line 364:
: TUpdateExecutorMembershipRequest update_req;
: for (const auto& it : snapshot->current_backends) {
: const TBackendDescriptor& backend = it.second;
: if (backend.is_executor) {
: update_req.hostnames.insert(backend.address.hostname);
: update_req.ip_addresses.insert(backend.ip_address);
: update_req.num_executors++;
: }
: }
: Status status = this->frontend()->UpdateExecutorMembership(update_req);
: if (!status.ok()) {
: LOG(WARNING) << "Error updating frontend membership snapshot: "
: << status.GetDetail();
: }
I'm inclined to think that this code should not be in the ExecEnv (but we can postpone that to a cleanup change). I'm aware that the typedefs inside the CMM cannot be forward-declared and we shouldn't include it in the frontend, but we should look into pulling the typedefs out of the CMM and then simplify this code by moving it into the destination of the callbacks.
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@494
PS8, Line 494: std::
nit: remove std:: in .cc files
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@496
PS8, Line 496: current_backend_set.insert(it.second.address);
Same as comment in the other callback
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/admission-controller.h@948
PS8, Line 948: load metric of the 'grp_name' executor group by 'delta'
While reading this I was confused what 'delta' is, maybe instead of "query load" we should use "query count" (load is often a value between 0 and 1)? 'num_queries' might also be a good parameter name if we rename the method to something like "IncExecGroupMetricLocked()".
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.h@124
PS8, Line 124: typedef std::function<void(SnapshotPtr)> UpdateCallbackFn;
Previously, UpdateLocalServerFn was only called when backends had been deleted. I think it would be nice to preserve that, e.g. by adding a bool can_contain_deletes (feel free to find a better name) to the callback.
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@321
PS8, Line 321: SetStateAndUpdateMetrics(new_state);
I know I asked for the metric update to be moved there, but now that we generalized the callback mechanism, I think it could be nice to update the metrics in a callback. What do you think?
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@322
PS8, Line 322:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@323
PS8, Line 323: NotifyAllCallbackUpdateFns
I think NotifyListeners(new_state) might be more concise but clear enough
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json@2502
PS6, Line 2502: ad.$0"
> do you think num-running-queries is more appropriate here?
maybe even num-running is ok (see my comment elsewhere about "load"). How do we call other metrics that track number of queries?
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 8
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Sep 2019 21:59:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14103
to look at the new patch set (#13).
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
IMPALA-8858: Add metrics tracking num queries running on executor groups
With this patch, all executor groups with at least one executor
will have a metric added that display the number of queries
(admitted by the local coordinator) running on them. The metric
is removed only when the group has no executors in it. It gets updated
when either the cluster membership changes or a query gets admitted or
released by the admission controller. Also adds the ability to delete
metrics from a metric group after registration.
Testing:
Added a custom cluster test and a BE metric test.
Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
---
M be/src/runtime/exec-env.cc
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_executor_groups.py
13 files changed, 342 insertions(+), 180 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/14103/13
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 13
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: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 11:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/4525/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 11
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 22:33:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 13: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14103/13/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/14103/13/be/src/scheduling/cluster-membership-mgr.h@68
PS13, Line 68: BlacklistExecutor
> nit: BlacklistExecutor()
Done
http://gerrit.cloudera.org:8080/#/c/14103/13/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:
http://gerrit.cloudera.org:8080/#/c/14103/13/tests/custom_cluster/test_executor_groups.py@385
PS13, Line 385: assert self.coordinator.service.get_metric_value(
> This pattern occurs quite often, maybe a helper self._get_num_executor_grou
Done
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 13
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 18:10:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8858: Add metrics tracking num queries running on executor groups
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add metrics tracking num queries running on executor groups
......................................................................
Patch Set 17: Code-Review+2
Carrying over Andrew's +2
--
To view, visit http://gerrit.cloudera.org:8080/14103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539
Gerrit-Change-Number: 14103
Gerrit-PatchSet: 17
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: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 20:09:54 +0000
Gerrit-HasComments: No