[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18178 ) Change subject: IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 30: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75cf17290be2c64fd4b732a5505bdac31869712a Gerrit-Change-Number: 18178 Gerrit-PatchSet: 30 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 21 Mar 2022 15:35:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has removed a vote on this change. Change subject: IMPALA-10992 Planner changes for estimate peak memory .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/18178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I75cf17290be2c64fd4b732a5505bdac31869712a Gerrit-Change-Number: 18178 Gerrit-PatchSet: 23 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18178 ) Change subject: IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 23: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75cf17290be2c64fd4b732a5505bdac31869712a Gerrit-Change-Number: 18178 Gerrit-PatchSet: 23 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 10 Mar 2022 17:24:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18178 ) Change subject: IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 19: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75cf17290be2c64fd4b732a5505bdac31869712a Gerrit-Change-Number: 18178 Gerrit-PatchSet: 19 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 08 Mar 2022 17:10:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18178 ) Change subject: IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 17: Code-Review+1 Looks good to me. Can you check with Kurt if he wants to do another review, otherwise I can give a +2. Also, just curious if running all tests with 2 group sets caused a significant increase in core or exhaustive test run time. If it is quite significant, we might want to rethink that strategy -- To view, visit http://gerrit.cloudera.org:8080/18178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75cf17290be2c64fd4b732a5505bdac31869712a Gerrit-Change-Number: 18178 Gerrit-PatchSet: 17 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Sat, 05 Mar 2022 02:45:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10999 Flakiness in TestAsyncLoadData.test async load
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18268 ) Change subject: IMPALA-10999 Flakiness in TestAsyncLoadData.test_async_load .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18268 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2ac954b0494b7413ce0ec405718fcc354dba9e0 Gerrit-Change-Number: 18268 Gerrit-PatchSet: 3 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 04 Mar 2022 22:25:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18178 ) Change subject: IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 16: (6 comments) Just a few more nits, everything else looks good http://gerrit.cloudera.org:8080/#/c/18178/16/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/18178/16/common/thrift/Frontend.thrift@734 PS16, Line 734: threshold nit: wondering if it makes sense to just call it max_mem_limit. This would make the variable name self explanatory http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1799 PS13, Line 1799: ExecutorGrou > query_exec_request.query_ctx.request_pool is set after we have decided the got it, thanks for the explanation http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1937 PS13, Line 1937: req.query_exec_request != nu > This is just a preventive check. Can this be converted into a Preconditions.check then? http://gerrit.cloudera.org:8080/#/c/18178/16/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/18178/16/fe/src/main/java/org/apache/impala/service/Frontend.java@1756 PS16, Line 1756: A group set is considered useless if its name is not a suffix :* of 'request_pool'. nit: mentioned the default executor group case as an exception http://gerrit.cloudera.org:8080/#/c/18178/16/fe/src/main/java/org/apache/impala/service/Frontend.java@1784 PS16, Line 1784: 64*MEGABYTE nit: wondering if this is low enough to always be rejected http://gerrit.cloudera.org:8080/#/c/18178/16/fe/src/main/java/org/apache/impala/service/Frontend.java@1787 PS16, Line 1787: "large" nit: would be good to also retain the name if one exists. Otherwise this would be set as a pool name which might get confusing if something else is expected -- To view, visit http://gerrit.cloudera.org:8080/18178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75cf17290be2c64fd4b732a5505bdac31869712a Gerrit-Change-Number: 18178 Gerrit-PatchSet: 16 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 04 Mar 2022 18:41:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10999 Flakiness in TestAsyncLoadData.test async load
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18268 ) Change subject: IMPALA-10999 Flakiness in TestAsyncLoadData.test_async_load .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/18268/2/tests/metadata/test_load.py File tests/metadata/test_load.py: http://gerrit.cloudera.org:8080/#/c/18268/2/tests/metadata/test_load.py@131 PS2, Line 131: test_async_load nit: can you add a comment on what this test is verifying? Is the intent to just do a sanity check or to also verify that 2 different behaviors are expected from sync and async mode? http://gerrit.cloudera.org:8080/#/c/18268/2/tests/metadata/test_load.py@199 PS2, Line 199: exec_end_state == running_state or exec_end_state == finished_state was the original intent of this test to ensure different behaviour? that is, sync => finished state, async => running state ? Since sync can now return either, do we need to check any other metric or behavior to verify the difference between the two? -- To view, visit http://gerrit.cloudera.org:8080/18268 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2ac954b0494b7413ce0ec405718fcc354dba9e0 Gerrit-Change-Number: 18268 Gerrit-PatchSet: 2 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 03 Mar 2022 19:35:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/18178 )
Change subject: IMPALA-10992 Planner changes for estimate peak memory
..
Patch Set 13:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/18178/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18178/13//COMMIT_MSG@9
PS13, Line 9: executor groups
nit: multiple executor group sets.
Change mentions of executor group to executor group sets where applicable in
the commit message. This is to stay consistent with the distinction between
executor groups vs group sets
http://gerrit.cloudera.org:8080/#/c/18178/13//COMMIT_MSG@74
PS13, Line 74: Almost all FE and BE tests are now run in the artificial two
: executor setup except a few where a specific cluster
configuration
: is desirable;
Please see my comment in Frontend.java about how we can ensure re-planning
code-path gets tested while ensuring the actual group set gets used.
http://gerrit.cloudera.org:8080/#/c/18178/13/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/18178/13/common/thrift/Frontend.thrift@729
PS13, Line 729: // The optional threshold for queries to run in an executor
group
nit: can you provide more context here as to what this threshold signifies,
like which resource is represents and how it helps the planner make decisions.
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@265
PS13, Line 265: ///
: // BEGIN: Data members used by auto-scaling
nit: would it make sense to put this inside a separate
State class?
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@284
PS13, Line 284: // The stmt table cache to be used by all iterations of
auto-scaling compilation.
nit: mention when it is set and when can it be reset
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@335
PS13, Line 335: auto-scaling
nit: not sure auto-scaling is the right term here, since we are not scaling
anything but rather assigning a query to the right group. Open to suggestions
for more semantically appropriate terms.
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1757
PS13, Line 1757: removes useless groups from it and fills the
:* threshold field
nit: would be good to explain why a group can be classified as useless and how
the threshold is evaluated
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1776
PS13, Line 1776: ExecutorMembershipSnapshot cluster =
ExecutorMembershipSnapshot.getCluster();
: int num_nodes = cluster.numExecutors();
: // Form a two-executor group testing environment so
that we can exercise
: // auto-scaling logic (see getTExecRequest() in
Frontend.java).
: TExecutorGroupSet r = new TExecutorGroupSet(num_nodes,
num_nodes, "regular");
: r.setThreshold(64*MEGABYTE);
: result.add(r);
: TExecutorGroupSet l = new TExecutorGroupSet(num_nodes,
num_nodes, "large");
: l.setThreshold(Long.MAX_VALUE);
: result.add(l);
if we want to emulate a 2 exec group set configuration, what if we only add a
dummy executor group set which will always be rejected (i think its the
'regular' one in this case) and the other one as the actual group set 'e =
executorGroupSets.get(0)'. Otherwise this can get very confusing if the someone
is running a test where test_replan is true by default (and is not aware of
this implication) on a different configuration but the plan always gets created
for a 3 node group set. This can especially be true for custom cluster tests.
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1788
PS13, Line 1788: executorGroupSets.get(0)
nit: can just use 'e' here too.
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1799
PS13, Line 1799: request_pool
if we use query_exec_request.query_ctx.request_pool instead of
queryOptions.getRequest_pool(), that should have the resolved request pool that
always has the "root." prefix
http://gerrit.cloudera.org:8080/#/c/18178/13/fe/src/main/java/org/apache/impala/service/Frontend.java@1816
PS13, Line 1816: throw new AnalysisException("Request pool '" + request_pool
: + "' does not name
[Impala-ASF-CR] IMPALA-10992 Planner changes for estimate peak memory - v1
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18143 ) Change subject: IMPALA-10992 Planner changes for estimate peak memory - v1 .. Patch Set 29: (20 comments) http://gerrit.cloudera.org:8080/#/c/18143/29//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18143/29//COMMIT_MSG@9 PS29, Line 9: a set of executor : groups nit: multiple executor group sets. Change mentions of executor group to executor group sets where applicable in the commit message. This is to stay consistent with the distinction between executor groups vs group sets http://gerrit.cloudera.org:8080/#/c/18143/29//COMMIT_MSG@25 PS29, Line 25: To facilitate testing, the patch imposes an artificial two executor : group setup in FE. This setup is enabled when 'enable_replan' is set : to 3 (testing) or RuntimeEnv.INSTANCE.isTestEnv() is true as in most : frontend tests. The artificial two executor groups are configured as : follows. Does this mean all tests are run in this configuration? All planner tests and end2end pytest are currently based on a minicluster or a custom cluster to emulate an actual cluster. Please see my comment in Frontend.java about how we can ensure re-planning code-path gets tested while ensuring the actual group set gets used. http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Frontend.thrift@729 PS29, Line 729: The optional threshold for queries to run in an executor group nit: can you provide more context here as to what this threshold signifies, like which resource is represents and how it helps the planner make decisions. http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Query.thrift@585 PS29, Line 585: TEnableReplanMode.TESTING shouldn't this be set to ON by default as per the comment? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@267 PS29, Line 267: /// : // BEGIN: Data members used by auto-scaling nit: would it make sense to put this inside a separate State class? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@286 PS29, Line 286: // The stmt table cache to be used by all iterations of auto-scaling compilation. nit: mention when it is set and when can it be reset http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@339 PS29, Line 339: auto-scaling nit: not sure auto-scaling is the right term here, since we are not scaling anything but rather assigning a query to the right group. Open to suggestions for more semantically appropriate terms. http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1763 PS29, Line 1763: setupThresholdsForGroupSets nit: add a comment about what it does http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1780 PS29, Line 1780: enable_replan == TEnableReplanMode.TESTING nit: also might be worth logging this while planning http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1787 PS29, Line 1787: TExecutorGroupSet l = new TExecutorGroupSet(3, 3, "large"); : l.setThreshold(Long.MAX_VALUE); : result.add(l); if we want to emulate a 2 exec group set configuration, what if we only add a dummy executor group set which will always be rejected (i think its the 'regular' one in this case) and the other one as the actual group set 'e = executorGroupSets.get(0)'. Otherwise this can get very confusing if the someone is running a test (and is not aware of this) on a different configuration but the plan always gets created for a 3 node group set. http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1792 PS29, Line 1792: executorGroupSets.get(0) nit: can just use 'e' here too. http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1801 PS29, Line 1801: request_pool if we use query_exec_request.query_ctx.request_pool instead of queryOptions.getRequest_pool(), that should have the resolved request pool that always has the "root." prefix http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1802 PS29, Line 1802: if (request_pool != nu
[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Thu, 13 Jan 2022 01:31:03 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11063: Add metrics to expose state of each executor group set
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[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
[Impala-ASF-CR] IMPALA-11068: Add tuning flag to reduce scanner thread launch.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18126 ) Change subject: IMPALA-11068: Add tuning flag to reduce scanner thread launch. .. Patch Set 1: Should we look into making the constant COMPRESSED_TEXT_COMPRESSION_RATIO tunable in hdfs-scan-node.cc instead? -- To view, visit http://gerrit.cloudera.org:8080/18126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I03cadf1230eed00d69f2890c82476c6861e37466 Gerrit-Change-Number: 18126 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Sat, 08 Jan 2022 03:26:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18093 ) Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. Patch Set 9: Code-Review+2 Saw 3 tests fail in the GVO, fixed the one that was related, others are unrelated flaky tests. IMPALA-10983, IMPALA-10927 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Sat, 08 Jan 2022 02:46:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Kurt Deschler, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#9). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size (the number of executors in each group). Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M tests/custom_cluster/test_executor_groups.py 12 files changed, 450 insertions(+), 67 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/9 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18093 ) Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 07 Jan 2022 18:09:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18093 ) Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@659 PS5, Line 659: EXPECT_EQ(update_req.exec_group_sets.size(), 1); : EXPECT_EQ(update_req.exec_group_sets[0].curr_num_executors, 1); : EXPECT_EQ(update_req.exec_group_sets[0].expected_num_executors, 20); : EXPECT_EQ(update_req.exec_group_sets[0].exec_group_name_prefix, ""); > not really. I think adding some extra assertions in this test is sufficient Those methods are part of ExecutorMembershipSnapshot which is a class in java and cannot be used here in a c++ unit test. http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.h@222 PS5, Line 222: expected_exec_group_sets > In this patch we have two objects defined: TExecutorGroupSets and ExpectedE agreed, changing both to TExecutorGroupSets. to provide more context, my initial attempt for this patch had used TExecutorGroupSets in both places and the only reason i switched to using standard lib classes was to avoid including a larger header file (gen-cpp/Frontend_types.h) in cluster-membership-mgr.h since cluster-membership-mgr is itself included in a bunch of places and could potentially inflate build times -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 22 Dec 2021 21:14:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Kurt Deschler, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#7). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size (the number of executors in each group). Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java 11 files changed, 447 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/7 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18093 ) Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/18093/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18093/5//COMMIT_MSG@11 PS5, Line 11: size(the number of executors in each group). Every executor group > Nit: need space after size Done http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@573 PS5, Line 573: first > nit. Can we add 'group_set_name' and 'expected_size' to ExpectedExecGroupSe just to clarify, the ExpectedExecGroupSets is actually a typedef of vector> which is where the 'first' and 'second' accessors come from (std::pair<>). (https://gerrit.cloudera.org/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.h) To be able to name the member variables I would have to create a new struct definition. Let me know if you would strongly prefer to do the alternative. http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@588 PS5, Line 588: // increasing order > nit. of expected group size. Done http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@615 PS5, Line 615: 4 > nit. duplicated case number (@587). Done http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@631 PS5, Line 631: Invalid input with no colon separator > nit title is not right. Done http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@659 PS5, Line 659: EXPECT_EQ(update_req.exec_group_sets.size(), 1); : EXPECT_EQ(update_req.exec_group_sets[0].curr_num_executors, 1); : EXPECT_EQ(update_req.exec_group_sets[0].expected_num_executors, 20); : EXPECT_EQ(update_req.exec_group_sets[0].exec_group_name_prefix, ""); > Can we add some assertions on methods numExecutors() and contains() for all just to clarify, do you want me to add separate JUnit tests that mimic these cases? http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.cc@622 PS5, Line 622: DCHECK(!prefix.empty() || exec_group_sets.size() == 1) > Should this be exec_group_sets.size() == 1 is there to ensure that if prefix is empty then it should only contain a single exec_group_set that is added in L613 http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.cc@629 PS5, Line 629: if (!it.second.IsHealthy()) continue; > Is this interesting enough to warrant a log message? if a group is not healthy it, we have visibility over it as it will show up on the metrics so I felt logging a warning here might be an overkill. this is why the way matching_exec_groups_found is incremented, it always accounts for a group regardless of whether it is healthy or not, so an unhealthy group will not be logged below http://gerrit.cloudera.org:8080/#/c/18093/5/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java: http://gerrit.cloudera.org:8080/#/c/18093/5/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@97 PS5, Line 97: String host = address.getHostname(); > Return false when exec_group_sets.size() > 0? exec_group_sets will always be non empty for any kind of update. If using 'default' exec group, the addresses will be populated and we should return those, but if using non-default exec groups this 'address' will be empty and false will be returned http://gerrit.cloudera.org:8080/#/c/18093/5/fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java File fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java: http://gerrit.cloudera.org:8080/#/c/18093/5/fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java@61 PS5, Line 61: int num > nit. Probably it is better to use two arguments (int expected_num_executors Done -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comme
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Kurt Deschler, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#6). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size (the number of executors in each group). Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java 11 files changed, 447 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/6 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/18093 )
Change subject: IMPALA-11033: Add support for specifying multiple executor
group sets
..
Patch Set 5:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@554
PS3, Line 554: /// This tests various valid and invalid cases that the parsing
logic in
> Add a description comment
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@638
PS3, Line 638: }
> Add a description of the test
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@644
PS3, Line 644: gflags::FlagSaver saver;
> This is my problem but when I see 'expected' in a test I think it means a v
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@664
PS3, Line 664: }
> This sets the flag for all subsequent tests?
forgot to remove that, removed
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@682
PS3, Line 682: ExecutorGroup exec_group("foo-group1", 1);
> Nit: with more executors?
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc@685
PS2, Line 685: parsed_group_prefixes.insert(group_prefix);
> Since FE will use ExecutorMembershipSnapshot.exec_group_sets to generate se
If we are planning to base the threshold on the resource pool configs then that
information can change dynamically whereas an update to FE only happens when
there is a change in the cluster state. Considering that, it would be better to
fetch the latest resource config while planning and letting the planner arrange
the list as it sees fit.
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@585
PS3, Line 585: /// include the hostnames and IP addresses in the update to the
frontend. For non-default
> Nit: addresses in
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@586
PS3, Line 586: /// executor groups, we assume that we will read data remotely
and will only send the
> Nit: will read
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@588
PS3, Line 588: /// specified we apply the aforementioned steps for each group
set.
> Nit: steps for
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@646
PS3, Line 646: LOG(WARNING) << "Some executor groups either do not match
expected group sets or "
> I wonder if this should be a WARNING level log?
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc@291
PS3, Line 291: "'expected_executor_group_sets' which is a more expressive
way of specifying "
> Maybe explain here how this overrides the value from num_expected_executors
Done
http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc@297
PS3, Line 297: "For eg. “prefix1:10”, this set will include executor groups
named like "
> Is there any warning printed in this case?
yes, as per your previous suggestion, this will now be logged at warning level
instead of info level
http://gerrit.cloudera.org:8080/#/c/18093/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java:
http://gerrit.cloudera.org:8080/#/c/18093/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@71
PS2, Line 71: exec_group_sets
> I think the only missing piece in TExecutorGroupSet is the threshold. If FE
The resource config file is on the local file system which is loaded, parsed
and cached into memory. Only changes to the local file trigger a change to the
cached config. So any number of fetches for the configs should be quite fast.
http://gerrit.cloudera.org:8080/#/c/18093/3/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java:
http://gerrit.cloudera.org:8080/#/c/18093/3/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@71
PS3, Line 71: private final List exec_group_sets_;
> Should this be exec_group_sets_ (i.e with a trailing underscore) ?
Done
--
To view, visit h
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Kurt Deschler, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#5). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size(the number of executors in each group). Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java 11 files changed, 439 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/5 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Kurt Deschler, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#4). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size. Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java 11 files changed, 439 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/4 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/18093 )
Change subject: IMPALA-11033: Add support for specifying multiple executor
group sets
..
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/runtime/exec-env.cc@178
PS2, Line 178: const ClusterMembershipMgr::ExpectedExecGroupSets&
expected_exec_group_sets,
> typedef for ExecGroupSets
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr-test.cc
File be/src/scheduling/cluster-membership-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr-test.cc@556
PS2, Line 556: ClusterMembershipMgr::ExpectedExecGroupSets
expected_exec_group_sets;
> use ExecGroupSets typedef here.
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr-test.cc@753
PS2, Line 753: EXPECT_EQ(update_req.exec_group_sets[0].curr_num_executors,
1);
> negative test for duplicate names or invalid parameters?
The inputs for PopulateExecutorMembershipRequest are from the cluster snapshot
and the ExpectedExecGroupSets which are already parsed and error checked, so
none of those checks added for it.
However adding a test for duplicate check for TestPopulateExpectedExecGroupSets
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h
File be/src/scheduling/cluster-membership-mgr.h:
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@173
PS2, Line 173: /// Returns a pointer to the static empty group reserved for
scheduling coord only
> use ExecGroupSets typedef
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@218
PS2, Line 218:
> It will be nice if the current #executors is stored in this data structure.
That details is extracted from the cluster snapshot and populated in
PopulateExecutorMembershipRequest
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@221
PS2, Line 221: /// Parses the --expected_executor_group_sets startup flag and
populates
> use ExecGroupSets typedef
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@229
PS2, Line 229:
> use ExecGroupSets typedef
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@229
PS2, Line 229:
> I wonder if we need to use 'expected' in the name. To me, the concept of a
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@285
PS2, Line 285: /// The frontend uses cluster membership information to
determine whether it expects the
> use ExecGroupSets typedef
Done
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc@685
PS2, Line 685: parsed_group_prefixes.insert(group_prefix);
> How do we deal with a common situation where a small number of exec groups
For the first part of your comment: FE is sent the max size of an executor
group among all groups in the set, so it would plan based off of that and does
not need to consider individual groups in the set separately. (See
PopulateExecutorMembershipRequest for how the update to FE is staged)
For the second part: This method is only used to parse the startup flag and
does not have cluster's current state. The reason I chose to sort here by
expected size is because in general multiple exec group sets would have a
non-trivial difference in their sizes and a min executor count for a group
which usually is set to 90% to be considered healthy will prevent situations
where exec group sets switch places in this order when an update is sent to FE.
For, eg
set1 -> expected size 10 -> min executors to consider healthy: 9
set2 -> 20 -> min executors to consider healthy: 18
http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc@688
PS2, Line 688: Substitute("Invalid executor group set format: $0",
group.ToString()));
> could either DCHECK first.second!=second.second or make strong order to han
added check for group prefix duplicates. However, allowing duplicates on the
expected number as there can be a use case (though unlikely, for eg, 2 group
sets with same group size but different defaults for resource configs like
mt_dop).
Let me know if you think we should be more stringent on these allowances.
http://gerrit.cloudera.org:8080/#/c/18093/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/18093/2/common/thrift/Frontend.thrift@716
PS2, Line 716: cur
> max or current?
updated the des
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Kurt Deschler, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#3). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size. Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java 11 files changed, 431 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/3 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11054: Support resource pool polling for frontend
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18078 ) Change subject: IMPALA-11054: Support resource pool polling for frontend .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia78b1a0574f6b8ad4df5bb0fc9533f218b486e6b Gerrit-Change-Number: 18078 Gerrit-PatchSet: 4 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 15 Dec 2021 18:34:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Hello Andrew Sherman, Qifan Chen, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18093 to look at the new patch set (#2). Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size. Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/ClusterSizeTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java 11 files changed, 411 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/2 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen
[Impala-ASF-CR] IMPALA-11054: Support resource pool polling for frontend
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18078 ) Change subject: IMPALA-11054: Support resource pool polling for frontend .. Patch Set 3: Code-Review+1 Looks good to me, deferring +2 to Qifan in case he has any more comments -- To view, visit http://gerrit.cloudera.org:8080/18078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia78b1a0574f6b8ad4df5bb0fc9533f218b486e6b Gerrit-Change-Number: 18078 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 13 Dec 2021 23:20:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11054: Support resource pool polling for frontend
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/18078 )
Change subject: IMPALA-11054: Support resource pool polling for frontend
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18078/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18078/2//COMMIT_MSG@9
PS2, Line 9: To support different sizes of resource pools, Planner will need to
poll
: resource pool which is only done by backend currently.
nit: can remove this context since its not relevant to the patch directly
http://gerrit.cloudera.org:8080/#/c/18078/2/fe/src/main/java/org/apache/impala/util/RequestPoolService.java
File fe/src/main/java/org/apache/impala/util/RequestPoolService.java:
http://gerrit.cloudera.org:8080/#/c/18078/2/fe/src/main/java/org/apache/impala/util/RequestPoolService.java@345
PS2, Line 345: if (LOG.isTraceEnabled()) {
: LOG.trace("resolveRequestPool(pool={}, user={}):
resolved_pool={}, has_access={}",
: resolvePoolParams.getRequested_pool(),
resolvePoolParams.getUser(),
: result.resolved_pool, result.has_access);
: }
maybe log this at the end right before return so that its logged in all cases
and not only when there is a successful resolution. (similar to how it was
logged every time before this patch/0
--
To view, visit http://gerrit.cloudera.org:8080/18078
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia78b1a0574f6b8ad4df5bb0fc9533f218b486e6b
Gerrit-Change-Number: 18078
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Comment-Date: Mon, 13 Dec 2021 17:41:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11033: Add support for specifying multiple executor group sets
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18093 Change subject: IMPALA-11033: Add support for specifying multiple executor group sets .. IMPALA-11033: Add support for specifying multiple executor group sets This patch introduces the concept of executor group sets. Each group set specifies an executor group name prefix and an expected group size. Every executor group that is a part of this set will have the same prefix which will also be equivalent to the resource pool name that it maps to. These sets are specified via a startup flag 'expected_executor_group_sets' which is a comma separated list in the format :. Testing: - Added unit tests Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 --- M be/src/runtime/exec-env.cc M be/src/scheduling/CMakeLists.txt M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java 9 files changed, 395 insertions(+), 54 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/18093/1 -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10764: hide /logs link in webui if --logtostderr=true
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18062 ) Change subject: IMPALA-10764: hide /logs link in webui if --logtostderr=true .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/18062/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18062/1//COMMIT_MSG@10 PS1, Line 10: --redirect_stdout_stderr=false > The default for redirect_stdout_stderr is true got it, thanks for the explanation -- To view, visit http://gerrit.cloudera.org:8080/18062 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65234213f32902caa1f4368181b49f012a4dbcb3 Gerrit-Change-Number: 18062 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 02 Dec 2021 01:01:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10764: hide /logs link in webui if --logtostderr=true
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18062 ) Change subject: IMPALA-10764: hide /logs link in webui if --logtostderr=true .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/18062/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18062/1//COMMIT_MSG@10 PS1, Line 10: --redirect_stdout_stderr=false is this also required to ensure no logs are generated? if yes, should we add a check on it too while removing the page? -- To view, visit http://gerrit.cloudera.org:8080/18062 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65234213f32902caa1f4368181b49f012a4dbcb3 Gerrit-Change-Number: 18062 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 01 Dec 2021 18:29:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10970: Fix criterion for classifying coordinator only query
Bikramjeet Vig has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17937 ) Change subject: IMPALA-10970: Fix criterion for classifying coordinator only query .. IMPALA-10970: Fix criterion for classifying coordinator only query This patch fixes a bug in the criterion which decided whether a query can be considered as a coordinator only query. It did not consider the possibility of parallel plans and ended up mis-classifying some queries as coordinator only queries. This classification was used during scheduling when dedicated coordinators and executor groups are used and allowed coordinator queries to be scheduled only on the coordinator even in the absence of healthy executor groups. As a result of this bug, queries classified wrongly ended up with error code: NO_REGISTERED_BACKENDS. Testing: - Add new mt_dop test case for functional_query and pass - Ran and passed custom_cluster/test_coordinators, test_executor_groups Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700 Reviewed-on: http://gerrit.cloudera.org:8080/17937 Tested-by: Impala Public Jenkins Reviewed-by: Bikramjeet Vig --- M be/src/scheduling/scheduler.cc M common/thrift/Query.thrift M testdata/workloads/functional-query/queries/QueryTest/mt-dop.test 3 files changed, 129 insertions(+), 2 deletions(-) Approvals: Impala Public Jenkins: Verified Bikramjeet Vig: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700 Gerrit-Change-Number: 17937 Gerrit-PatchSet: 5 Gerrit-Owner: guojingfeng Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: guojingfeng
[Impala-ASF-CR] IMPALA-10970: Fix criterion for classifying coordinator only query
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17937 ) Change subject: IMPALA-10970: Fix criterion for classifying coordinator only query .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700 Gerrit-Change-Number: 17937 Gerrit-PatchSet: 4 Gerrit-Owner: guojingfeng Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: guojingfeng Gerrit-Comment-Date: Tue, 30 Nov 2021 20:12:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11035: Make x-forwarded-for http header case-insensitive
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18048 ) Change subject: IMPALA-11035: Make x-forwarded-for http header case-insensitive .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9c4070a4a2d5ad9decb186a9219957d8c26a7d7 Gerrit-Change-Number: 18048 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 24 Nov 2021 03:33:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11035: Make x-forwarded-for http header case-insensitive
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18048 ) Change subject: IMPALA-11035: Make x-forwarded-for http header case-insensitive .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/18048/1/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/18048/1/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@396 PS1, Line 396: "X-Forwarded-For" nit: maybe just add a separate simple test like Case2 above (mentioning that it checks specifically for case-insensitivity) to make it obvious what we are testing -- To view, visit http://gerrit.cloudera.org:8080/18048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9c4070a4a2d5ad9decb186a9219957d8c26a7d7 Gerrit-Change-Number: 18048 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 23 Nov 2021 21:04:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10970: Fix criterion for classifying coordinator only query
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/17937 )
Change subject: IMPALA-10970: Fix criterion for classifying coordinator only
query
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.h@77
PS3, Line 77: /// If there are only one TPlanExecInfo, single unpartitioned
fragment is considered
: /// coordinator only.
: /// If there are multiple TPlanExecInfo, we should check all of
them is co-located in
: /// coordinator node. If all other TPlanExecInfo has only one
fragments and it's
: /// partition type is unpartitioned, we should also take this
Query is a coordinator
: /// only. (e.g. JOIN BUILD sink (MT_DOP>0) is co-located with
its pre order ancestor
: /// : JOIN_NODE) Check IMPALA-4224 for more detail.
nit: I think the original comment is good enough, this is adding extra context
for an edge case which is an implementation detail and does not need to me
mentioned in the method comment
http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.cc@824
PS3, Line 824: if (exec_request.plan_exec_info.size() > 1) {
: /// There are intermediate join build side plan that
co-located with join node,
: /// don't take the query as coordinator only.
: return false;
: }
we can instead do something like,
num_parallel_plans = exec_request.plan_exec_info.size();
then at the end check for num_parallel_plans == 1, this way it makes the code
self explanatory and helps us avoid adding comments. In addition to this, to
further improve readability, you can add a better description to
'plan_exec_info' member in struct TQueryExecRequest in Query.thrift file as
follows:
'The first plan in the list materializes the query result and subsequent plans
materialize the build sides of joins. Each plan appears before its dependencies
in the list.'
http://gerrit.cloudera.org:8080/#/c/17937/3/testdata/workloads/functional-query/queries/QueryTest/mt-dop.test
File testdata/workloads/functional-query/queries/QueryTest/mt-dop.test:
http://gerrit.cloudera.org:8080/#/c/17937/3/testdata/workloads/functional-query/queries/QueryTest/mt-dop.test@30
PS3, Line 30: # TIMPALA-128: Coordinator only query determination error.
: # From IMPALMA-4224 we execute join build in a separate
fragments, the root
: # plan contains two subplan, one of it contains join node while
the other
: # contains join builder as its root, the separate join build
fragment is
: # co-located with join node.
: # This test case will fail due to outdated coordinator only
determination
: # logic which only take only the first subplan for concern.
: # To ensure the inline left table placed at join probe side, we
add
: # STRAIGHT_JOIN hint to disable join inversion optimization.
How about?
IMPALA-10970: Verify that a query plan containing 2 parallel plans with the
first one having a single unpartitioned fragment is correctly classified as not
being a coordinator only query. This query create a plan with the
aforementioned condition and has a join build fragment that will be co-located
with the root sink fragment(IMPALMA-4224). This ensures that the query fails to
schedule and eventually times out in the queue if incorrectly classified as a
coordinator only query.
To ensure the inline left table placed at join probe side, we add STRAIGHT_JOIN
hint to disable join inversion optimization.
--
To view, visit http://gerrit.cloudera.org:8080/17937
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700
Gerrit-Change-Number: 17937
Gerrit-PatchSet: 3
Gerrit-Owner: guojingfeng
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: guojingfeng
Gerrit-Comment-Date: Fri, 19 Nov 2021 00:43:04 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10970: Update coordinator only judgment logic to adapt separate join build execution
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17937 ) Change subject: IMPALA-10970: Update coordinator only judgment logic to adapt separate join build execution .. Patch Set 1: Just wanted to check if you got a chance to work on the next iteration of this patch -- To view, visit http://gerrit.cloudera.org:8080/17937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700 Gerrit-Change-Number: 17937 Gerrit-PatchSet: 1 Gerrit-Owner: guojingfeng Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Nov 2021 17:44:19 + Gerrit-HasComments: No
[Impala-ASF-CR] [WIP] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17994 ) Change subject: [WIP] IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 11: > I was thinking that the default and large group can be partitioned > among two disjoint sets of nodes in a cluster. Thus each should > have their own host names/ ip addresses for the scans (in > particular local reads) to work properly. > > The update on these two groups is contained in > TUpdateExecutorMembershipRequest > (https://gerrit.cloudera.org/#/c/17994/11/common/thrift/Frontend.thrift). > It should still be one message. When using executor groups (i.e., non-default executor group name) ip addresses and hostnames are not sent in the updates (as the comment above PopulateExecutorMembershipRequest suggests). So all reads are assumed as remote reads and those ip addresses and hostnames will never be sent. -- To view, visit http://gerrit.cloudera.org:8080/17994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dca6933f7db3d9e00b20c93b38310b0e77a09eb Gerrit-Change-Number: 17994 Gerrit-PatchSet: 11 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Comment-Date: Tue, 16 Nov 2021 21:08:06 + Gerrit-HasComments: No
[Impala-ASF-CR] [WIP] IMPALA-10992 Planner changes for estimate peak memory
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17994 ) Change subject: [WIP] IMPALA-10992 Planner changes for estimate peak memory .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/17994/11/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/17994/11/be/src/service/impala-server.h@525 PS11, Line 525: LARGE_EXECUTOR_GROUP_NAME I went through the patch briefly and it seems like the patch is based around the idea that there can be 2 sets of executor group types in the "default" mode. ‘default’ here is a bit of a misnomer. The “DEFAULT_EXECUTOR_GROUP_NAME” is used when we are not using the new concept of multiple executor groups at all. This is the legacy mode where there is a single cluster and all executors belong to that. So we dont need to specify a different “LARGE_EXECUTOR_GROUP_NAME” name since a large executor group can only exist when using “executor groups”. The same concept carries over to the changes in cluster-membership-mgr and ExecutorMembershipSnapshot. We dont need to send separate updates for large and regular executor groups because when we are using executor-groups, fields like hostnames and ip_addresses are not passed on. We only need ‘num_executors’. Therefore we can avoid keeping 2 sets of hostnames and ip_addresses and only need another set of num_executors. This will greatly simplify the update code. -- To view, visit http://gerrit.cloudera.org:8080/17994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dca6933f7db3d9e00b20c93b38310b0e77a09eb Gerrit-Change-Number: 17994 Gerrit-PatchSet: 11 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Comment-Date: Tue, 16 Nov 2021 02:16:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has removed a vote on this change. Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17891 ) Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Patch Set 4: Hit another flaky test IMPALA-11012 -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Tue, 09 Nov 2021 20:42:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has removed a vote on this change. Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-10970: Update coordinator only judgment logic to adapt separate join build execution
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17937 ) Change subject: IMPALA-10970: Update coordinator only judgment logic to adapt separate join build execution .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/17937/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17937/1//COMMIT_MSG@7 PS1, Line 7: Update coordinator only judgment logic to adapt separate j Fix criterion for classifying coordinator only query http://gerrit.cloudera.org:8080/#/c/17937/1//COMMIT_MSG@9 PS1, Line 9: IMPALA-8830 introduce new executor group assignment for coordinator only : queries which assign all coordinator queries to a 'empty' executor group : that we got the ability to execute trivil queries regardless of the : presence of any healthy executor groups. : : IsCoordinatorOnlyQuery is introduced to determine query is coordinator : only or not. Before this change, IsCoordinatorOnlyQuery only take the : first TPlanExecInfo of TQueryExecRequest for concern. : : This is not enough since IMPALA-4224 execute separate join builds : fragments for parallel join execution. With the new join execution mode, : join builder is executed in separated plan if MT_DOP > 0. This means we : will have 2 TPlanExecInfo for a single join node, join builder is : co-located with join node but at separate TPlanExecInfo. So without this : patch the following query will fail in admission because we will take : it as coordinator only query. In debug build it will hit the DECHECK at : /be/src/scheduling/scheduler.cc#L162, in release build it will : fail with: "Scheduling for executor group: empty group : (using coordinator only) with 0 executors". : : This patch update the judgment logic to take all of TPlanExecInfo for : concern. nit: This patch fixes a bug in the criterion which decided whether a query can be considered as a coordinator only query. It did not consider the possibility of parallel plans and ended up mis-classifying some queries as coordinator only queries. This classification was used during scheduling when dedicated coordinators and executor groups are used and allowed coordinator queries to be scheduled only on the coordinator even in the absence of healthy executor groups. As a result of this bug, queries classified wrongly ended up being queued and timed-out waiting. http://gerrit.cloudera.org:8080/#/c/17937/1/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/17937/1/be/src/scheduling/scheduler.cc@a824 PS1, Line 824: Thanks for catching this bug. A stronger check here would be to ensure that there is a single element in the plan_exec_info list. Can you please add a check for plan_exec_info.size() == 1. To give you more context the only way there can be more plans in plan_exec_info is if they produce intermediate join build sides in a parallel plan. Since in those cases they cannot be small coordinator only queries it would make sense to check for that condition as well. http://gerrit.cloudera.org:8080/#/c/17937/1/testdata/workloads/functional-query/queries/QueryTest/mt-dop.test File testdata/workloads/functional-query/queries/QueryTest/mt-dop.test: http://gerrit.cloudera.org:8080/#/c/17937/1/testdata/workloads/functional-query/queries/QueryTest/mt-dop.test@39 PS1, Line 39: SELECT : STRAIGHT_JOIN t1.ts,count(t2.l_orderkey) : FROM : ( SELECT '20210831' AS ts : UNION ALL SELECT '20210901' AS ts : UNION ALL SELECT '20210902' AS ts : UNION ALL SELECT '20210903' AS ts ) t1 : CROSS JOIN tpch.lineitem t2 : GROUP BY t1.ts : ORDER BY t1.ts; can you use the following query instead to bring down the runtime. (using tpch.lineitem takes ~ 7s vs functional.alltypes which takes ~ 0.17s) SELECT STRAIGHT_JOIN t2.int_col FROM ( SELECT '20210831' AS ts UNION ALL SELECT '20210901' AS ts UNION ALL SELECT '20210902' AS ts UNION ALL SELECT '20210903' AS ts ) t1 CROSS JOIN functional.alltypes t2 LIMIT 100 -- To view, visit http://gerrit.cloudera.org:8080/17937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700 Gerrit-Change-Number: 17937 Gerrit-PatchSet: 1 Gerrit-Owner: guojingfeng Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 04 Nov 2021 20:00:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17891 ) Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Patch Set 3: Hit another unrelated failure IMPALA-10886 -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Tue, 02 Nov 2021 20:07:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17891 ) Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Patch Set 3: unrelated flaky tests failed in last GVO, starting another one. -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Mon, 01 Nov 2021 22:56:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has removed a vote on this change. Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17891 ) Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. Patch Set 2: Code-Review+2 (4 comments) Carrying over Andrew's +2 http://gerrit.cloudera.org:8080/#/c/17891/1/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/17891/1/tests/custom_cluster/test_executor_groups.py@610 PS1, Line 610: > flake8: E122 continuation line missing indentation or outdented Done http://gerrit.cloudera.org:8080/#/c/17891/1/tests/custom_cluster/test_executor_groups.py@617 PS1, Line 617: " > flake8: E251 unexpected spaces around keyword / parameter equals Done http://gerrit.cloudera.org:8080/#/c/17891/1/tests/custom_cluster/test_executor_groups.py@620 PS1, Line 620: " > flake8: E251 unexpected spaces around keyword / parameter equals Done http://gerrit.cloudera.org:8080/#/c/17891/1/tests/custom_cluster/test_executor_groups.py@650 PS1, Line 650: s > flake8: F841 local variable 'handle_long_running_queue2' is assigned to but Done -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Fri, 29 Oct 2021 23:08:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Hello Andrew Sherman, Abhishek Rawat, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17891 to look at the new patch set (#2). Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. IMPALA-10943: Add test to verify support for multiple resource and executor pools This patch adds a test to verify that admission control accounting works when using multiple coordinators and multiple executor groups mapped to different resource pools and having different sizes. Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e --- M tests/custom_cluster/test_executor_groups.py 1 file changed, 86 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/17891/2 -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-10973: Do not schedule empty scan nodes to coordinator
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17954 ) Change subject: IMPALA-10973: Do not schedule empty scan nodes to coordinator .. Patch Set 3: @Csaba It would good to have a test case that verified this behavior -- To view, visit http://gerrit.cloudera.org:8080/17954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie31df3861aad2e3e91cab621ff122a4f721905ef Gerrit-Change-Number: 17954 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Oct 2021 21:49:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10973: Do not schedule empty scan nodes to coordinator
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17954 ) Change subject: IMPALA-10973: Do not schedule empty scan nodes to coordinator .. Patch Set 2: Had started GVO but then noticed that a GVO had already succeeded. So instead will just submit the changes -- To view, visit http://gerrit.cloudera.org:8080/17954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie31df3861aad2e3e91cab621ff122a4f721905ef Gerrit-Change-Number: 17954 Gerrit-PatchSet: 2 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Oct 2021 19:59:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10973: Do not schedule empty scan nodes to coordinator
Bikramjeet Vig has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17954 ) Change subject: IMPALA-10973: Do not schedule empty scan nodes to coordinator .. IMPALA-10973: Do not schedule empty scan nodes to coordinator Until now fragments with scan nodes that have no scan ranges were scheduled to the coordinator, even if it is an exclusive coordinator. This could possibly lead to a lot of work to be scheduled to the coordinator. This patch changes the logic to choose a random executor instead. Change-Id: Ie31df3861aad2e3e91cab621ff122a4f721905ef Reviewed-on: http://gerrit.cloudera.org:8080/17954 Tested-by: Impala Public Jenkins Reviewed-by: Abhishek Rawat Reviewed-by: Bikramjeet Vig --- M be/src/scheduling/scheduler.cc M be/src/scheduling/scheduler.h 2 files changed, 17 insertions(+), 14 deletions(-) Approvals: Impala Public Jenkins: Verified Abhishek Rawat: Looks good to me, but someone else must approve Bikramjeet Vig: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ie31df3861aad2e3e91cab621ff122a4f721905ef Gerrit-Change-Number: 17954 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10973: Do not schedule empty scan nodes to coordinator
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17954 ) Change subject: IMPALA-10973: Do not schedule empty scan nodes to coordinator .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie31df3861aad2e3e91cab621ff122a4f721905ef Gerrit-Change-Number: 17954 Gerrit-PatchSet: 2 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Oct 2021 19:57:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 08 Oct 2021 20:13:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: FragmentState* fragment_state = state->obj_pool()->Add( > You're absolutely right! We explicitly set ENABLE_EXPR_REWRITES=0 in patch can you add context here about the kind of plan generated? maybe just the textual plan itself? http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@78 PS4, Line 78: ity class nit: how about EnableCodegen(true) http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@629 PS4, Line 629: col", was the previous case not valid? http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@104 PS5, Line 104: new RuntimeState(query_ctx, &exec_env_); you can also allocate this from pool_ so that it can be cleaned eventually http://gerrit.cloudera.org:8080/#/c/17894/5/be/src/benchmarks/expr-benchmark.cc@171 PS5, Line 171: test_data does this ever get deleted? -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 07 Oct 2021 01:35:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 ) Change subject: IMPALA-10950: Update expr-benchmark.cc .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: const TQueryExecRequest& query_request = request.query_exec_request; > This one seems to be a bigger obstacle for benchmarking expression. you can use this to query option to disable re-write: ENABLE_EXPR_REWRITES -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 05 Oct 2021 19:42:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10942: Fix memory leak in admission controller
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17893 ) Change subject: IMPALA-10942: Fix memory leak in admission controller .. Patch Set 4: GVO failed earlier due to flaky test. Re-running -- To view, visit http://gerrit.cloudera.org:8080/17893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 Gerrit-Change-Number: 17893 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Rorke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: abeltian Gerrit-Comment-Date: Tue, 05 Oct 2021 18:18:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10942: Fix memory leak in admission controller
Bikramjeet Vig has removed a vote on this change. Change subject: IMPALA-10942: Fix memory leak in admission controller .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/17893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 Gerrit-Change-Number: 17893 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Rorke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: abeltian
[Impala-ASF-CR] IMPALA-10950: Update expr-benchmark.cc
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc
..
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@9
PS3, Line 9: Query planner has move the scalar expression's thrift definition
from
: 'fragments[0].output_sink.output_exprs' to
: 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]'.
which change made this switch? curious as to what the context there was
http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@16
PS3, Line 16: some expressions
: in the benchmark now will crash without codegen due to missing
symbols
why are they now crashing if the benchmark earlier used to run fine without
codegen?
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108
PS3, Line 108: const TQueryExecRequest& query_request =
request.query_exec_request;
nit: might be worth adding context here about the kind of plan created for a
constant query
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@119
PS3, Line 119: // Uncomment the following lines for debugging.
: // union_node.printTo(cout);
: // cout << endl << " Need Codegen: " <<
test->fragment_state->ScalarExprNeedsCodegen()
: // << endl << endl;
do these need to be removed?
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@134
PS3, Line 134: codegen->EnableOptimizations(false);
why are we disabling this?
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141
PS3, Line 141: void ReleaseTestData() {
: test_data_.clear();
: mem_pool_.FreeAll();
: }
nit: you can probably just add this to the destructor instead of a separate
function
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@183
PS3, Line 183: #define BENCHMARK(name, stmt) \
: suite->AddBenchmark(name, BenchmarkQueryFn,
GenerateBenchmarkExprs(stmt, true))
we should probably do both with and without codegen and also keep the benchmark
results for both, will be good to have historical data.
http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@234
PS3, Line 234: // Cast: FunctionRate
Comparison
: //
--
: // int_to_int 824
1X
: //int_to_bool 878
1.066X
: // int_to_double 775.4
0.941X
: // int_to_string 32.47
0.03941X
: // double_to_boolean 823.5
0.9994X
: // double_to_bigint 775.4
0.941X
: // double_to_string 4.682
0.005682X
: // string_to_int 402.6
0.4886X
: //string_to_float 145.8
0.1769X
: //string_to_timestamp 83.76
0.1017X
all of these values are for benchmark without codegen, can you update these too?
--
To view, visit http://gerrit.cloudera.org:8080/17894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f
Gerrit-Change-Number: 17894
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Tue, 05 Oct 2021 07:01:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10942: Fix memory leak in admission controller
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17893 ) Change subject: IMPALA-10942: Fix memory leak in admission controller .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 Gerrit-Change-Number: 17893 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Rorke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: abeltian Gerrit-Comment-Date: Mon, 04 Oct 2021 16:41:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10942: Fix memory leak in admission controller
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17893 ) Change subject: IMPALA-10942: Fix memory leak in admission controller .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 Gerrit-Change-Number: 17893 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Rorke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: abeltian Gerrit-Comment-Date: Mon, 04 Oct 2021 16:41:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10942: Fix memory leak in admission controller
Hello Andrew Sherman, abeltian, Riza Suminto, David Rorke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17893 to look at the new patch set (#3). Change subject: IMPALA-10942: Fix memory leak in admission controller .. IMPALA-10942: Fix memory leak in admission controller This patch fixes a memory leak in the admission controller where objects tracking state required for queuing were being retained despite the query being admitted or rejected immediately. Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 --- M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/local-admission-control-client.cc 4 files changed, 6 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/17893/3 -- To view, visit http://gerrit.cloudera.org:8080/17893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 Gerrit-Change-Number: 17893 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: David Rorke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: abeltian
[Impala-ASF-CR] IMPALA-10942: Fix memory leak in admission controller
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17893 Change subject: IMPALA-10942: Fix memory leak in admission controller .. IMPALA-10942: Fix memory leak in admission controller This patch fixes a memory leak in the admission controller where objects tracking state required for queuing where being retained despite the query being admitted or rejected immediately. Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 --- M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/local-admission-control-client.cc 4 files changed, 6 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/17893/2 -- To view, visit http://gerrit.cloudera.org:8080/17893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1 Gerrit-Change-Number: 17893 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10943: Add test to verify support for multiple resource and executor pools
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17891 Change subject: IMPALA-10943: Add test to verify support for multiple resource and executor pools .. IMPALA-10943: Add test to verify support for multiple resource and executor pools This patch adds a test to verify that admission control accounting works when using multiple coordinators and multiple executor groups mapped to different resource pools and having different sizes. Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e --- M tests/custom_cluster/test_executor_groups.py 1 file changed, 86 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/17891/1 -- To view, visit http://gerrit.cloudera.org:8080/17891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If76d386d8de5730da937674ddd9a69aa1aa1355e Gerrit-Change-Number: 17891 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-9930 (part 2): Introduce new admission control rpc service
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/16412 ) Change subject: IMPALA-9930 (part 2): Introduce new admission control rpc service .. Patch Set 14: (1 comment) http://gerrit.cloudera.org:8080/#/c/16412/14/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16412/14/be/src/scheduling/admission-controller.cc@1155 PS14, Line 1155: if (!queued) queue_nodes_.erase(request.query_id); > We can get a memory leak here. You are absolutely right! I have created a ticket for this IMPALA-10942 and will get a fix for it in soon. Thanks a lot for catching it. -- To view, visit http://gerrit.cloudera.org:8080/16412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I594fc593a27b24b6952e381a9bc1a9a5c6b757ae Gerrit-Change-Number: 16412 Gerrit-PatchSet: 14 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: abeltian Gerrit-Comment-Date: Thu, 30 Sep 2021 20:14:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10784 (part 3): Prepare to publish impala-shell on PyPi
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17826 ) Change subject: IMPALA-10784 (part 3): Prepare to publish impala-shell on PyPi .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/17826 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I378404e2407396d4de3bb0eea4d49a9c5bb4e46a Gerrit-Change-Number: 17826 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Wed, 15 Sep 2021 18:52:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9976 IMPALA-10866: Add recovery mechanism to admission service and fix consistency between coord failure detection and registration
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17332 ) Change subject: IMPALA-9976 IMPALA-10866: Add recovery mechanism to admission service and fix consistency between coord failure detection and registration .. Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG@9 PS1, Line 9: > I wrote my own version of the commit message to see that I understand the c Pretty much. The only thing I would add is that if a backend is marked as down by the statestore, it will also have to register(send full admission state) again with the admissiond to be able to be serviced http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG@12 PS1, Line 12: - Leverages the admission heartbeat mechanism to signal the > Nit" should this be "No RPCs are serviced from a coordinator until it has s Done http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-client.cc File be/src/scheduling/admission-control-client.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-client.cc@32 PS1, Line 32: "Re-submit for admission due to a possible admission service restart or network " > Is there a reason it is only a "possible" restart? as this can also be due to a generic network error that prevents the Admit RPC to go through. Added this to the error msg. Open to suggestion for a better message text http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc File be/src/scheduling/admission-control-service.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc@304 PS1, Line 304: LOG(INFO) << "Received heartbeat from unrecognized coord_id=" << req->coord_id(); > Maybe WARNING is too high, we do expect to see this after a restart, maybe Just a log line at the INFO level should suffice in that case http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc@332 PS1, Line 332: bool registered_on_ac_service = > Is it possible that the admissionstate we have just received is more up-to- I have made both the RebuildAdmissionState and CancelQueriesOnFailedCoordinators atomic operations so there should be no inconsistency anymore. Also if the coord is already registered it would return an OK status. In that case as well there should be no inconsistency as all RPCs after the first successful call to RebuildAdmissionState would be serviced and ideally any subsequent calls for RebuildAdmissionState should just be previous retries. http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc@449 PS1, Line 449: lock_guard l(admission_state->lock); > The size of known_coord_ids_ is interesting, maybe add it to the message. Done http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-controller.cc@2103 PS1, Line 2103: DCHECK( > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-controller.cc@2104 PS1, Line 2104: num_backends_to_release_.find(state->query_id()) == num_backends_to_release_.end()); > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.h File be/src/scheduling/remote-admission-control-client.h: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.h@54 PS1, Line 54: /// TODO: add info on what this does? here or in the class comments > Yes it seems like these methods don't have descriptions will add these in the next update http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.cc File be/src/scheduling/remote-admission-control-client.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.cc@131 PS1, Line 131: // retry_admission can be false if AttemptAdmissionAndWait succeeded but a > I think this was already set to true? This might not be set to true if the RPC succeeded but the impala-server initiated a ResetPendingAdmit. This can happen in the following case: - the RPCs (both admit and getQueryStatus) succeeded on a previous instance of the admissiond - The coordinator re-registered with an ongoing (old or a new restarted) instance of the admissiond. http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.cc@158 PS1, Line 158: while (admit_rpc_status.IsNetworkError() > Nit: "admissiond" Done http://gerrit.cloudera.org:8080/#/c/17332/1/common/protobuf/admission_control_service.proto File common/protobuf/admission_control_service.proto: http://gerrit.cl
[Impala-ASF-CR] IMPALA-9976 IMPALA-10866: Add recovery mechanism to admission service and fix consistency between coord failure detection and registration
Hello Andrew Sherman, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17332 to look at the new patch set (#2). Change subject: IMPALA-9976 IMPALA-10866: Add recovery mechanism to admission service and fix consistency between coord failure detection and registration .. IMPALA-9976 IMPALA-10866: Add recovery mechanism to admission service and fix consistency between coord failure detection and registration Major changes: IMPALA-9976: - Leverages the admission heartbeat mechanism to signal the coordinator to send its complete admission state - No RPCs are serviced by a coordinator until it has sent its complete admission state. This is to prevent making admission decisions till admission service has built its view of the cluster - The complete admission state consists of the states of all queries that have successfully been admitted, that is, received a valid schedule from the admission controller and have marked its admission as complete (for remote admission it means its pending admit status has transitioned from true to false) - This helps prevent sending incomplete/inconsistent state to the admission controller - Queries that have not started admission get a chance to send their request to the new service - Queries that are queued restart the admission process by sending the request again. This re-try is now also marked in the query profile - Other RPCs like ReleaseBackend, ReleaseQuery, CancelQuery that don't get serviced (till initial admission state is sent) can result in inconsistent state. This state will be rectified in the admission heartbeats - AdmitQuery and GetQueryStatus just retry again if they notice a network failure(assuming admissiond might be down/restarting) or received the error message that they cannot be serviced yet admissiond is waiting on initial state from this coordinator) IMPALA-10866: - Made sure that admission state removal on failure detection and admission state rebuilding on coordinator registration are atomic operations. - Leverage statestore's membership view to detect failure and allow coordinator registration. Limitations: - Rebuilding the state can not ensure that queued queries will maintain their spot in the queue. - Queries can be admitted before all coordinators get a chance to send their state. This can result in a brief period of over-admission We cannot rely completely on the statestore membership update and wait for all coordinators there to send admission state because that membership is also dynamic which makes it difficult to decide when to assume that the admission state is complete. - The functionalities for coordinator failure detection and registration rely completely on the statestore. Testing: - Added end to end tests Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd --- M be/src/runtime/coordinator-backend-resource-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/scheduling/admission-control-client.cc M be/src/scheduling/admission-control-client.h M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h 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/admissiond-env.cc M be/src/scheduling/local-admission-control-client.cc M be/src/scheduling/local-admission-control-client.h M be/src/scheduling/remote-admission-control-client.cc M be/src/scheduling/remote-admission-control-client.h M be/src/scheduling/schedule-state.cc M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/protobuf/admission_control_service.proto M common/thrift/generate_error_codes.py M tests/custom_cluster/test_admission_controller.py 24 files changed, 756 insertions(+), 93 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/17332/2 -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-10784 (part 2): Fix retaining cookies for impala-shell
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17796 ) Change subject: IMPALA-10784 (part 2): Fix retaining cookies for impala-shell .. Patch Set 3: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/17796/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17796/3//COMMIT_MSG@11 PS3, Line 11: Pythonic coding Adding Attila to verify changes are in line with python best practices -- To view, visit http://gerrit.cloudera.org:8080/17796 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65432b952929c1c96a081bb87fd4a096624d711b Gerrit-Change-Number: 17796 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 20 Aug 2021 22:04:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10874: Upgrade impyla to the latest version
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17795 ) Change subject: IMPALA-10874: Upgrade impyla to the latest version .. Patch Set 1: Code-Review+1 Looks good to me. @Attila will the version changes in requirements.txt for dependent libs like sasl or bitarray affect other dependencies? -- To view, visit http://gerrit.cloudera.org:8080/17795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I990e5cdde4e98d6ab3581fe48f53a5d0590ce492 Gerrit-Change-Number: 17795 Gerrit-PatchSet: 1 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 19 Aug 2021 23:05:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10720: Add versioning to admission heartbeats
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17524 ) Change subject: IMPALA-10720: Add versioning to admission heartbeats .. Patch Set 3: Code-Review+2 Carrying forward +2 from Andrew -- To view, visit http://gerrit.cloudera.org:8080/17524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d Gerrit-Change-Number: 17524 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Tue, 17 Aug 2021 00:59:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10720: Add versioning to admission heartbeats
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17524 ) Change subject: IMPALA-10720: Add versioning to admission heartbeats .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/17524/2/be/src/scheduling/admission-control-service.h File be/src/scheduling/admission-control-service.h: http://gerrit.cloudera.org:8080/#/c/17524/2/be/src/scheduling/admission-control-service.h@139 PS2, Line 139: /// from it. NOTE: Can contain stale data from coordinators that have restarted. > Nit: "from" Done http://gerrit.cloudera.org:8080/#/c/17524/2/common/protobuf/admission_control_service.proto File common/protobuf/admission_control_service.proto: http://gerrit.cloudera.org:8080/#/c/17524/2/common/protobuf/admission_control_service.proto@274 PS2, Line 274: /// resources. Stale heartbeat messages are ignored. > Nit: "heartbeat" Done -- To view, visit http://gerrit.cloudera.org:8080/17524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d Gerrit-Change-Number: 17524 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Tue, 17 Aug 2021 00:59:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10720: Add versioning to admission heartbeats
Hello Andrew Sherman, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17524 to look at the new patch set (#3). Change subject: IMPALA-10720: Add versioning to admission heartbeats .. IMPALA-10720: Add versioning to admission heartbeats This patch adds version numbers to admission heartbeats which allows the admission service to ignore stale (out of order) heartbeats from coordinators. Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d --- M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/protobuf/admission_control_service.proto 5 files changed, 39 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/17524/3 -- To view, visit http://gerrit.cloudera.org:8080/17524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d Gerrit-Change-Number: 17524 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] IMPALA-10783: Fixed flakiness in run and verify query cancellation test
Hello Csaba Ringhofer, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17768 to look at the new patch set (#2). Change subject: IMPALA-10783: Fixed flakiness in run_and_verify_query_cancellation_test .. IMPALA-10783: Fixed flakiness in run_and_verify_query_cancellation_test The issue was that after the impala-shell is started in a seperate process and an error is encountered then the process lingers on and a long running query can hold on to resources and potentially affect other tests running on the impala cluster. This patch just makes sure that the impala-shell process is killed regardless of any errors encountered. Change-Id: I9f6d22d639921051cde5675fae1845bedb61c8cc --- M tests/shell/test_shell_commandline.py 1 file changed, 8 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/17768/2 -- To view, visit http://gerrit.cloudera.org:8080/17768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f6d22d639921051cde5675fae1845bedb61c8cc Gerrit-Change-Number: 17768 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer
[Impala-ASF-CR] IMPALA-10783: Fixed flakiness in run and verify query cancellation test
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17768 ) Change subject: IMPALA-10783: Fixed flakiness in run_and_verify_query_cancellation_test .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/17768/1/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/17768/1/tests/shell/test_shell_commandline.py@387 PS1, Line 387: try: > Having this line in the try block doesn't seem useful to me, as the finally Done http://gerrit.cloudera.org:8080/#/c/17768/1/tests/shell/test_shell_commandline.py@423 PS1, Line 423: > Same as line 387 Done -- To view, visit http://gerrit.cloudera.org:8080/17768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f6d22d639921051cde5675fae1845bedb61c8cc Gerrit-Change-Number: 17768 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Comment-Date: Tue, 17 Aug 2021 00:55:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10846: Skip Authentication for connection with trusted auth header
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/17759 )
Change subject: IMPALA-10846: Skip Authentication for connection with trusted
auth header
..
Patch Set 1:
(1 comment)
Looks good, Andrew's covered most of the stuff, just one more nit from my side
http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.cc
File be/src/util/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.cc@907
PS1, Line 907: bool Webserver::HandleTrustedAuthHeader(
: struct sq_connection* connection, struct sq_request_info*
request_info) {
: string err_msg;
: if (!GetUsernameFromAuthHeader(connection, request_info,
err_msg)) {
: LOG(ERROR) << "Found trusted auth header but " << err_msg;
: return false;
: }
: return true;
: }
nit: you can just get rid of HandleTrustedAuthHeader if you are only calling
GetUsernameFromAuthHeader.
--
To view, visit http://gerrit.cloudera.org:8080/17759
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7103012bc9cf9dfdc9c184af3a281526d8127a31
Gerrit-Change-Number: 17759
Gerrit-PatchSet: 1
Gerrit-Owner: Wenzhe Zhou
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Tue, 10 Aug 2021 00:30:12 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17683 ) Change subject: IMPALA-8762: Track host level admission stats across all coordinators .. Patch Set 3: Code-Review+2 Carrying over Andrew's +2 -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 27 Jul 2021 23:22:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17683 ) Change subject: IMPALA-8762: Track host level admission stats across all coordinators .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/17683/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/17683/2/be/src/scheduling/admission-controller.cc@257 PS2, Line 257: << "All admission topic key prefixes should be of the same size"; > Nit: "All admission topic keys" is better, as this may make it clearer for Done http://gerrit.cloudera.org:8080/#/c/17683/2/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/17683/2/tests/custom_cluster/test_executor_groups.py@573 PS2, Line 573: # identical but this will at least test that code path as a sanity check. > Nit: "at least" "code path" Done -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 27 Jul 2021 23:22:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators
Hello Andrew Sherman, Joe McDonnell, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17683 to look at the new patch set (#3). Change subject: IMPALA-8762: Track host level admission stats across all coordinators .. IMPALA-8762: Track host level admission stats across all coordinators This patch adds the ability to share the per-host stats for locally admitted queries across all coordinators. This helps to get a more consolidated view of the cluster for stats like slots_in_use and mem_admitted when making local admission decisions. Testing: Added e2e py test Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 --- M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/StatestoreService.thrift M common/thrift/metrics.json M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_catalog_wait.py M tests/custom_cluster/test_executor_groups.py M tests/custom_cluster/test_runtime_profile.py M tests/custom_cluster/test_scratch_disk.py 9 files changed, 261 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/17683/3 -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17683 ) Change subject: IMPALA-8762: Track host level admission stats across all coordinators .. Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/17683/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17683/1//COMMIT_MSG@7 PS1, Line 7: coordinators > nit: coordinators Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h@440 PS1, Line 440: // This maps a backends's id(host/port id) to its host level statistics which are used > Nit: Its not a struct any more Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h@442 PS1, Line 442: kends. > nit: what's the map key? host_id? Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@75 PS1, Line 75: // "". "!" is used because the backend id > Nit: is it clearer to say it is of the form? Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@244 PS1, Line 244: // Parses the topic key to separate the prefix that helps recognize the kind of update > Nit: separate Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@245 PS1, Line 245: // received. > Nit: received Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@1652 PS1, Line 1652: if (topic_backend_id == host_id_) continue; > This is skipping updates about the current host? this skips the update that was added by the current host in its previous statestore update, since the current host's local version is more up to date. Similar to what we do in L1635 above http://gerrit.cloudera.org:8080/#/c/17683/1/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/17683/1/tests/custom_cluster/test_executor_groups.py@96 PS1, Line 96: self.num_impalads = 2 > why is this 2? this is a mis-nomer, it basically refers to the num of impalad instances that self._start_impala_cluster will look for. See L66 and L80. I'll update the naming of the variables to represent their true meaning. -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 20 Jul 2021 20:50:27 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all coordinators
Hello Andrew Sherman, Joe McDonnell, Wenzhe Zhou, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17683 to look at the new patch set (#2). Change subject: IMPALA-8762: Track host level admission stats across all coordinators .. IMPALA-8762: Track host level admission stats across all coordinators This patch adds the ability to share the per-host stats for locally admitted queries across all coordinators. This helps to get a more consolidated view of the cluster for stats like slots_in_use and mem_admitted when making local admission decisions. Testing: Added e2e py test Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 --- M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/StatestoreService.thrift M common/thrift/metrics.json M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_catalog_wait.py M tests/custom_cluster/test_executor_groups.py M tests/custom_cluster/test_runtime_profile.py M tests/custom_cluster/test_scratch_disk.py 9 files changed, 261 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/17683/2 -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-8762: Track host level admission stats across all cooridnators
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17683 Change subject: IMPALA-8762: Track host level admission stats across all cooridnators .. IMPALA-8762: Track host level admission stats across all cooridnators This patch adds the ability to share the per-host stats for locally admitted queries across all cordinators. This helps to get a more consolidated view of the cluster for stats like slots_in_use and mem_admitted when making local admission decisions. Testing: Added e2e py test Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 --- M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/StatestoreService.thrift M common/thrift/metrics.json M tests/custom_cluster/test_executor_groups.py 5 files changed, 242 insertions(+), 68 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/17683/1 -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10784: Add support for retaining cookies in impala-shell
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17667 ) Change subject: IMPALA-10784: Add support for retaining cookies in impala-shell .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17667 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I193422d5ec891886a522d82ecb0e9d974132ff2a Gerrit-Change-Number: 17667 Gerrit-PatchSet: 5 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 12 Jul 2021 16:15:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10784: Add support for retaining cookies in impala-shell
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17667 ) Change subject: IMPALA-10784: Add support for retaining cookies in impala-shell .. Patch Set 3: (4 comments) Looks good, just a few nits http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py File shell/ImpalaHttpClient.py: http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@66 PS3, Line 66: http_cookie_names is comma-separated list of cookie names which are used to specify : the cookie-based authentication or session management nit: http_cookie_names is used to specify a comma-separated list of possible cookie names used for cookie-based authentication or session management. If a cookie with one of these names is returned in an http response by the server or an intermediate proxy then it will be included in each subsequent request for the same connection. http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@170 PS3, Line 170: updateHttpCookie nit: updateHttpCookies http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@184 PS3, Line 184: self.__http_cookie_dict[cn] = Cookie(cookie=None, expiry_time=None) nit: can we just remove the entry from __http_cookie_dict? http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@194 PS3, Line 194: addHttpCookie nit: addHttpCookiesToRequestHeaders -- To view, visit http://gerrit.cloudera.org:8080/17667 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I193422d5ec891886a522d82ecb0e9d974132ff2a Gerrit-Change-Number: 17667 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 09 Jul 2021 19:39:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10557: Support Kudu's multi-row transaction
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17553 ) Change subject: IMPALA-10557: Support Kudu's multi-row transaction .. Patch Set 21: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17553 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I876ada48991afdff5d61b5d6a0417571aba7cb34 Gerrit-Change-Number: 17553 Gerrit-PatchSet: 21 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 24 Jun 2021 14:32:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10557: Support Kudu's multi-row transaction
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17553 ) Change subject: IMPALA-10557: Support Kudu's multi-row transaction .. Patch Set 20: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/17553/20/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/17553/20/common/thrift/Query.thrift@684 PS20, Line 684: // Stores the transaction id if the query is transactional. nit: update to mention that this is only used for HIVE ACID http://gerrit.cloudera.org:8080/#/c/17553/20/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/17553/20/tests/custom_cluster/test_kudu.py@540 PS20, Line 540: patch nit: batch -- To view, visit http://gerrit.cloudera.org:8080/17553 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I876ada48991afdff5d61b5d6a0417571aba7cb34 Gerrit-Change-Number: 17553 Gerrit-PatchSet: 20 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 24 Jun 2021 00:38:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10762: ASAN tests fail with use-after-poison in HdfsParquetScanner::FindSkipRangesForPagesWithMinMaxFilters
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17630 ) Change subject: IMPALA-10762: ASAN tests fail with use-after-poison in HdfsParquetScanner::FindSkipRangesForPagesWithMinMaxFilters .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97c43571d771805834577a0f03d476e24c82b82b Gerrit-Change-Number: 17630 Gerrit-PatchSet: 1 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 23 Jun 2021 20:18:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10720: Add versioning to admission heartbeats
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17524 ) Change subject: IMPALA-10720: Add versioning to admission heartbeats .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/17524/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17524/1//COMMIT_MSG@10 PS1, Line 10: allows the admission service to ignore stale (out of order) > Maybe clarify that stale here means out of order? Done http://gerrit.cloudera.org:8080/#/c/17524/1/be/src/scheduling/admission-control-service.h File be/src/scheduling/admission-control-service.h: http://gerrit.cloudera.org:8080/#/c/17524/1/be/src/scheduling/admission-control-service.h@138 PS1, Line 138: /// Maps from coordinator ID to the latest heartbeat version number that was processed > Nit: "coordinator" Done http://gerrit.cloudera.org:8080/#/c/17524/1/be/src/scheduling/admission-control-service.h@140 PS1, Line 140: /// TODO: Leverage IMPALA-9155 to add coord_id the first time a coord sends a heartbeat > I think this map will contains stale data form coordinators that have resta Added a TODO to add a mechanism to delete ids when coords fail http://gerrit.cloudera.org:8080/#/c/17524/1/be/src/scheduling/admission-control-service.cc File be/src/scheduling/admission-control-service.cc: http://gerrit.cloudera.org:8080/#/c/17524/1/be/src/scheduling/admission-control-service.cc@276 PS1, Line 276: VLOG(1) << "Stale heartbeat received for coord_id: "<< req->host_id(); > Maybe should also say that this may be expected if a coordinator restarts. if a coord restarts it'll receive a new Id when it registers with the statestore. So this case can only happen due to network issues. -- To view, visit http://gerrit.cloudera.org:8080/17524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d Gerrit-Change-Number: 17524 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Mon, 21 Jun 2021 22:18:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10720: Add versioning to admission heartbeats
Hello Andrew Sherman, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17524 to look at the new patch set (#2). Change subject: IMPALA-10720: Add versioning to admission heartbeats .. IMPALA-10720: Add versioning to admission heartbeats This patch adds version numbers to admission heartbeats which allows the admission service to ignore stale (out of order) heartbeats from coordinators. Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d --- M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/protobuf/admission_control_service.proto 5 files changed, 39 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/17524/2 -- To view, visit http://gerrit.cloudera.org:8080/17524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d Gerrit-Change-Number: 17524 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell
[Impala-ASF-CR] WIP IMPALA-10557: Support Kudu's multi-row transaction
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/17553 )
Change subject: WIP IMPALA-10557: Support Kudu's multi-row transaction
..
Patch Set 5:
(9 comments)
Looks good, just adding a few more nits
http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.h
File be/src/exec/kudu-table-sink.h:
http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.h@107
PS5, Line 107: /// Returns TRUE if it's in Kudu transaction.
nit: add: valid only after Open() succeeds
http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/exec/kudu-table-sink.h@123
PS5, Line 123: std::string kudu_txn_token_
> nit: is it possible to add 'const' specifier and initialize the token in th
Deserialize can return an error status which we cant propagate during
initialization, therefore anything like this is usually deferred to the Open()
call
http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/17553/5/be/src/service/client-request-state.cc@1323
PS5, Line 1323: if (InTransaction()) {
: AbortTransaction();
: } else if (InKuduTransaction()) {
: AbortKuduTransaction();
: }
: LOG(ERROR) << "ERROR Finalizing DML: " <<
status.GetDetail();
: return status;
: }
: if (InTransaction()) {
: // UpdateCatalog() succeeded and already committed the
transaction for us.
: int64_t txn_id = GetTransactionId();
: if (!frontend_->UnregisterTransaction(txn_id).ok()) {
: LOG(ERROR) << Substitute("Failed to unregister
transaction $0", txn_id);
: }
: ClearTransactionState();
: query_events_->MarkEvent("Transaction committed");
: } else if (InKuduTransaction()) {
: CommitKuduTransaction();
: }
wondering if we can consolidate the methods for Transactions and let them take
care of doing the right thing for hive and kudu. will make it easier to read.
eg L1331-L1341 can be taken care of by FinalizeTransaction
L1323-L1327 can be taken care of by a single AbortTransaction()
http://gerrit.cloudera.org:8080/#/c/17553/5/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/17553/5/common/thrift/ImpalaService.thrift@673
PS5, Line 673: KUDU_TRANSACTION
nit: how about ENABLE_KUDU_TRANSACTION
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@1124
PS5, Line 1124: setTransactionToken
nit: maybe call it setKuduTransactionToken
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/planner/TableSink.java
File fe/src/main/java/org/apache/impala/planner/TableSink.java:
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/planner/TableSink.java@110
PS5, Line 110:* Same as above, plus it takes an ACID write id in parameter
'writeId'.
nit: update comment to mention txnToken
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/planner/TableSink.java@116
PS5, Line 116: txnToken
nit: maybe name it kuduTxnToken to explicitly specify that this is kudu specific
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1732
PS5, Line 1732: TransactionException
update comment for TransactionException to include kudu cases too.
http://gerrit.cloudera.org:8080/#/c/17553/5/fe/src/main/java/org/apache/impala/service/Frontend.java@2242
PS5, Line 2242: txn.close();
nit: is close() something that can be put in a finally block. Like should it
always be called even it rollback or commit fails? I understand that it is
Auto-closeable but if we are calling it explicitly here and not using the
try-with-resource code construct then might be good to just put it in the
finally block.
--
To view, visit http://gerrit.cloudera.org:8080/17553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I876ada48991afdff5d61b5d6a0417571aba7cb34
Gerrit-Change-Number: 17553
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Revie
[Impala-ASF-CR] IMPALA-10720: Add versioning to admission heartbeats
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17524 Change subject: IMPALA-10720: Add versioning to admission heartbeats .. IMPALA-10720: Add versioning to admission heartbeats This patch adds version numbers to admission heartbeats which allows the admission service to ignore stale heartbeats from coordinators. Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d --- M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/protobuf/admission_control_service.proto 5 files changed, 37 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/17524/1 -- To view, visit http://gerrit.cloudera.org:8080/17524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1338211dc0bca67f8fde93a1b05c0780be583d5d Gerrit-Change-Number: 17524 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-9155: Add recovery mechanism to admission service
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17332 Change subject: IMPALA-9155: Add recovery mechanism to admission service .. IMPALA-9155: Add recovery mechanism to admission service Major changes: - Leverages the admission heartbeat mechanism to signal the coordinator to send its complete admission state - No RPCs are serviced by a coordinator unless it sends its complete admission state. This is to prevent making admission decisions till admission service has built its view of the cluster - The complete admission state consists of the states of all queries that have successfully been admitted, that is, received a valid schedule from the admission controller and have marked its admission as complete (for remote admission it means its pending admit status has transitioned from true to false) - This helps prevent sending incomplete/inconsistent state to the admission controller - Queries that have not started admission get a chance to send their request to the new service - Queries that are queued restart the admission process by sending the request again. This re-try is now also marked in the query profile - Other RPCs like ReleaseBackend, ReleaseQuery, CancelQuery that don't get serviced (till initial admission state is sent) can result in inconsistent state. This state will be rectified in the admission heartbeats - AdmitQuery and GetQueryStatus just retry again if they notice a network failure(assuming admissiond might be down/restarting) or received the error message that they cannot be serviced yet admissiond is waiting on initial state from this coordinator) Limitations: - Rebuilding the state can not ensure that queued queries will maintain their spot in the queue. - Queries can be admitted before all coordinators get a chance to send their state. This can result in a brief period of over-admission We cannot rely completely on the statestore membership update and wait for all coordinators there to send admission state because that membership is also dynamic which makes it difficult to decide when to assume that the admission state is complete. Testing: - Added end to end tests Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd --- M be/src/runtime/coordinator-backend-resource-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/scheduling/admission-control-client.cc M be/src/scheduling/admission-control-client.h M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h 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/local-admission-control-client.cc M be/src/scheduling/local-admission-control-client.h M be/src/scheduling/remote-admission-control-client.cc M be/src/scheduling/remote-admission-control-client.h M be/src/scheduling/schedule-state.cc M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/protobuf/admission_control_service.proto M common/thrift/generate_error_codes.py M tests/custom_cluster/test_admission_controller.py 23 files changed, 674 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/17332/1 -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10596: De-flake TestAdmissionControllerStress
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17272 Change subject: IMPALA-10596: De-flake TestAdmissionControllerStress .. IMPALA-10596: De-flake TestAdmissionControllerStress Currently some tests in TestAdmissionControllerStress fail because they rely on queries holding onto resources on backends till they are explicitly closed. This was done by keeping the query alive by fetching very limited rows in intervals. After results spooling was turned on by default, the queries started to finish early and released their resources on other backends which let queued queries to get admitted. This patch turns off result spooling for the queries to avoid this situation. Testing: Ran test in a loop on my local dev machine. Change-Id: I1594a82f507db8dc094d4441dd8739d037f974ff --- M tests/custom_cluster/test_admission_controller.py 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/17272/1 -- To view, visit http://gerrit.cloudera.org:8080/17272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1594a82f507db8dc094d4441dd8739d037f974ff Gerrit-Change-Number: 17272 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10596: De-flake teardown in TestAdmissionControllerStress
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17256 Change subject: IMPALA-10596: De-flake teardown in TestAdmissionControllerStress .. IMPALA-10596: De-flake teardown in TestAdmissionControllerStress Currently if the threads running queries in TestAdmissionControllerStress hit an error, they close their client which ultimately closes the query that it was running. If teardown() runs after the client is closed, it tries to cancel the query that the thread was running and hits an exception trying to cancel an already closed query. This results in the pytest throws the exception encountered in teardown() instead of the original exception that caused the test to fail in the first place. This patch fixes this by removing the query handle from the thread if the client is closed. Testing: Simulated hitting an error condition in the main thread that initially triggered this condition. Change-Id: I8aa8315d9f598ba80d13cd2091e3cc743c64ba77 --- M tests/custom_cluster/test_admission_controller.py 1 file changed, 7 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/17256/1 -- To view, visit http://gerrit.cloudera.org:8080/17256 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8aa8315d9f598ba80d13cd2091e3cc743c64ba77 Gerrit-Change-Number: 17256 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10397: De-flake test single workload
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17239 Change subject: IMPALA-10397: De-flake test_single_workload .. IMPALA-10397: De-flake test_single_workload This patch removes a flaky part of the test that relies on query completion rate. Since we are already verifying that number of healthy executor groups increases, this additional check is not adding much to the test. Change-Id: I6f75afdbe676d9dd6922b6ba8aa1919daa161947 --- M tests/custom_cluster/test_auto_scaling.py 1 file changed, 3 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/17239/1 -- To view, visit http://gerrit.cloudera.org:8080/17239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I6f75afdbe676d9dd6922b6ba8aa1919daa161947 Gerrit-Change-Number: 17239 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10397: Fix test single workload
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17218 ) Change subject: IMPALA-10397: Fix test_single_workload .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/17218/1/tests/custom_cluster/test_auto_scaling.py File tests/custom_cluster/test_auto_scaling.py: http://gerrit.cloudera.org:8080/#/c/17218/1/tests/custom_cluster/test_auto_scaling.py@49 PS1, Line 49: # two comparisons and should take around 2 second to complete. > I think this will now be 2 seconds? Done -- To view, visit http://gerrit.cloudera.org:8080/17218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide3c7fb4509ce9a797b4cbdd141b2a319b923d4e Gerrit-Change-Number: 17218 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 25 Mar 2021 00:15:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10397: Fix test single workload
Hello Andrew Sherman, Abhishek Rawat, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17218 to look at the new patch set (#2). Change subject: IMPALA-10397: Fix test_single_workload .. IMPALA-10397: Fix test_single_workload The logs on failed runs indicated that the autoscaler never started another cluster. This can only happen if it never notices a queued query which is possible since this test was only failing in release builds. This patch increases the runtime of the sample query to make execution more predictable. Testing: Looped on my local on a release build Change-Id: Ide3c7fb4509ce9a797b4cbdd141b2a319b923d4e --- M tests/custom_cluster/test_auto_scaling.py 1 file changed, 6 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/17218/2 -- To view, visit http://gerrit.cloudera.org:8080/17218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ide3c7fb4509ce9a797b4cbdd141b2a319b923d4e Gerrit-Change-Number: 17218 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10397: Fix test single workload
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17218 Change subject: IMPALA-10397: Fix test_single_workload .. IMPALA-10397: Fix test_single_workload The logs on failed runs indicated that the autoscaler never started another cluster. This can only happen if it never notices a queued query which is possible since this test was only failing in release builds. This patch increases the runtime of the sample query to make execution more predictable. Testing: Looped on my local on a release build Change-Id: Ide3c7fb4509ce9a797b4cbdd141b2a319b923d4e --- M tests/custom_cluster/test_auto_scaling.py 1 file changed, 5 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/17218/1 -- To view, visit http://gerrit.cloudera.org:8080/17218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ide3c7fb4509ce9a797b4cbdd141b2a319b923d4e Gerrit-Change-Number: 17218 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-10594: Handle failed coordinators in admissiond
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17209 ) Change subject: IMPALA-10594: Handle failed coordinators in admissiond .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/17209/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17209/2//COMMIT_MSG@13 PS2, Line 13: > There won't be any overadmission because the executors also monitor the sta Got it. Thanks for the explanation. -- To view, visit http://gerrit.cloudera.org:8080/17209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I883f323bb765680ef24b3c3f51fb209dea15f0b0 Gerrit-Change-Number: 17209 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 22 Mar 2021 20:08:54 + Gerrit-HasComments: Yes
