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 2022/01/12 04:18:41 UTC

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

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


Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................

IMPALA-11063: Add metrics to expose state of each executor group set

This adds metrics for each executor group set that expose the number
of executor groups, the number of healthy executor groups and the
total number of backends associated with that group set.

Testing:
Added an e2e test to verify metrics are updated correctly.

Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
---
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
4 files changed, 162 insertions(+), 27 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 2: Code-Review+1

(2 comments)

Thanks!

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@89
PS1, Line 89: MetricGroup* metric_grp = metrics->GetOrCreateChildGroup("cluster-membership");
            :   aggregated_group_set_metrics_.total_live_executor_groups_ =
            :       metric_grp->AddCounter(LIVE_EXEC_GROUP_KEY, 0);
            :   aggregated_group_set_metrics_.total_healthy_executor_groups_ =
            :       metric_grp->AddCounter(HEALTHY_EXEC_GROUP_KEY, 0);
> The keys and inputs to AddCounter() are different for 'overall' vs 'per_gro
Done


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@606
PS1, Line 606: ngPiece name(grou
> group.NumHosts() > 0 is the only simple check we need unlike IsHealthy() wh
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jan 2022 00:09:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 1: Code-Review+1

(2 comments)

LGTM, see small worry

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@64
PS1, Line 64: static const string LIVE_EXEC_GROUP_KEY_FORMAT(
Nit: This is not incorrect, but "cluster-membership.executor-groups.total.$0" shares a prefix with "cluster-membership.executor-groups.total" and that's the sort of thing that could tickle autoscaler bugs where code uses prefixes to fetch metrics.


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

http://gerrit.cloudera.org:8080/#/c/18142/1/tests/custom_cluster/test_executor_groups.py@134
PS1, Line 134:       metric_name += "." + exec_group_set_prefix
This is where the code I marked with a Nit is actually useful



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 18:04:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

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/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................

IMPALA-11063: Add metrics to expose state of each executor group set

This adds metrics for each executor group set that expose the number
of executor groups, the number of healthy executor groups and the
total number of backends associated with that group set.

Testing:
Added an e2e test to verify metrics are updated correctly.

Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Reviewed-on: http://gerrit.cloudera.org:8080/18142
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
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
4 files changed, 172 insertions(+), 29 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7751/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jan 2022 03:11:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 2: Code-Review+2

LGTM


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jan 2022 01:50:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.h@248
PS1, Line 248: aggregated_group_set_m
> nit. May name it as overall_group_set_metrics_ to keep it consistent with p
Done


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@64
PS1, Line 64: static const string LIVE_EXEC_GROUP_KEY_FORMAT(
> Nit: This is not incorrect, but "cluster-membership.executor-groups.total.$
Done


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@89
PS1, Line 89: MetricGroup* metric_grp = metrics->GetOrCreateChildGroup("cluster-membership");
            :   aggregated_group_set_metrics_.total_live_executor_groups_ =
            :       metric_grp->AddCounter(LIVE_EXEC_GROUP_KEY, 0);
            :   aggregated_group_set_metrics_.total_healthy_executor_groups_ =
            :       metric_grp->AddCounter(HEALTHY_EXEC_GROUP_KEY, 0);
> nit.these lines of code can be moved to a method GroupSetMetrics::Add(Metri
The keys and inputs to AddCounter() are different for 'overall' vs 'per_group_set' so the special casing for both would end up with the same structure as InitMetrics() here.


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@606
PS1, Line 606: ngPiece name(grou
> I wonder if there is a method ExecutorGroup::isLive(). If so, it can be use
group.NumHosts() > 0 is the only simple check we need unlike IsHealthy() which needs to know the 'min_size' of the exec group as well (where 'min size' is the minimum number of executors in the group to be considered healthy)


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@611
PS1, Line 611: 
             :       if (group.NumHosts() > 0) {
> nit. duplicated lines with the THEN branch.
Good point. Done.


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

http://gerrit.cloudera.org:8080/#/c/18142/1/tests/custom_cluster/test_executor_groups.py@134
PS1, Line 134:         metric_name = "cluster-membership.executor-groups.total"
> This is where the code I marked with a Nit is actually useful
Done


http://gerrit.cloudera.org:8080/#/c/18142/1/tests/custom_cluster/test_executor_groups.py@685
PS1, Line 685: c
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/18142/1/tests/custom_cluster/test_executor_groups.py@694
PS1, Line 694: _
> flake8: E251 unexpected spaces around keyword / parameter equals
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jan 2022 01:31:03 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/18142/1/tests/custom_cluster/test_executor_groups.py@685
PS1, Line 685:  
flake8: E251 unexpected spaces around keyword / parameter equals


http://gerrit.cloudera.org:8080/#/c/18142/1/tests/custom_cluster/test_executor_groups.py@694
PS1, Line 694:  
flake8: E251 unexpected spaces around keyword / parameter equals



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 04:19:43 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Qifan Chen, Wenzhe Zhou, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................

IMPALA-11063: Add metrics to expose state of each executor group set

This adds metrics for each executor group set that expose the number
of executor groups, the number of healthy executor groups and the
total number of backends associated with that group set.

Testing:
Added an e2e test to verify metrics are updated correctly.

Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
---
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
4 files changed, 172 insertions(+), 29 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jan 2022 03:11:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jan 2022 09:46:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 1:

(4 comments)

Looks good to me.

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.h@248
PS1, Line 248: overall_group_metrics_
nit. May name it as overall_group_set_metrics_ to keep it consistent with per_group_set_metrics_.


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@89
PS1, Line 89: overall_group_metrics_.total_live_executor_groups_ =
            :       metric_grp->AddCounter(LIVE_EXEC_GROUP_KEY, 0);
            :   overall_group_metrics_.total_healthy_executor_groups_ =
            :       metric_grp->AddCounter(HEALTHY_EXEC_GROUP_KEY, 0);
            :   overall_group_metrics_.total_backends_ = metric_grp->AddCounter(TOTAL_BACKENDS_KEY, 0);
nit.these lines of code can be moved to a method GroupSetMetrics::Add(MetricGroup)


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@606
PS1, Line 606: group.IsHealthy()
I wonder if there is a method ExecutorGroup::isLive(). If so, it can be useful to manipulate total_live_exec_groups.


http://gerrit.cloudera.org:8080/#/c/18142/1/be/src/scheduling/cluster-membership-mgr.cc@611
PS1, Line 611: ++total_live_exec_groups;
             :         total_backends += group.NumExecutors();
nit. duplicated lines with the THEN branch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 20:23:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10007/ : 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/18142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jan 2022 01:54:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18142 )

Change subject: IMPALA-11063: Add metrics to expose state of each executor group set
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10004/ : 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/18142
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib39f940de830ef6302785aee30eeb847fa5deeba
Gerrit-Change-Number: 18142
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: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 04:42:00 +0000
Gerrit-HasComments: No