You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/08/21 19:52:08 UTC
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7760
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
13 files changed, 95 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/1
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7760
to look at the new patch set (#5).
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the IP address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch also takes into account of a problem found in
IMPALA-5795. In particular, the backend descriptor of the
coordinator may not be found in the backend map in the
scheduler if coordinator is not an executor (i.e. dedicated
coordinator). The fix to also check against the local backend
descriptor.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
16 files changed, 158 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/5
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7760
to look at the new patch set (#7).
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the IP address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch also takes into account of a problem found in
IMPALA-5795. In particular, the backend descriptor of the
coordinator may not be found in the backend map in the
scheduler if coordinator is not an executor (i.e. dedicated
coordinator). The fix to also check against the local backend
descriptor.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
16 files changed, 158 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/7
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 96: local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr);
> Oh, I was confused by all the different places and ways we set the port and
Yeah, I'm in favour of pulling this from the ExecEnv if possible, just because flags are such an indirect interface; it's hard to tell what depends on them.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0);
> I don't think there's a hard and fast rule. In general I think it's not a b
As far as I understand, the only other user of this constructor (other than ExecEnv::ExecEnv()) is in-process-server.cc. In that case, it's already setting the FLAGS_be_port in InProcessImpalaServer::StartWithEphemeralPorts(), right ? In addition, there are quite a number of places in the code which access FLAGS_*_port directly so we have a leaky encapsulation anyway.
How about we simplify things by merging the two constructors of ExecEnv::ExecEnv() and use FLAGS directly in c'tor ?
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 96: local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr);
> why does that have to get saved in the backend descriptor? since the port i
We can replace krpc_svc_address with krpc_svc_port as the port can be different (in theory) for different hosts. Using krpc_svc_address instead of krpc_svc_port is easier as it requires assembling it at one place instead of spreading it out to multiple call sites. That said, it's not terribly expensive to assemble the address either. It just seems more consistent with the existing
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
Line 1660: MakeNetworkAddress(ip, FLAGS_krpc_port));
> another reason to not add krpc_svc_address but instead just generate it fro
Either way seems fine to me but we still need to propagate the port number if not the entire address. So, it's a matter of shifting MakeNetworkAddress() to the consumers or creating it once here in the producer. Please let me know if you have a strong preference.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 5: Code-Review+1
Carry +1
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the IP address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch also takes into account of a problem found in
IMPALA-5795. In particular, the backend descriptor of the
coordinator may not be found in the backend map in the
scheduler if coordinator is not an executor (i.e. dedicated
coordinator). The fix to also check against the local backend
descriptor.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Reviewed-on: http://gerrit.cloudera.org:8080/7760
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
16 files changed, 158 insertions(+), 60 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Michael Ho: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0);
Add a TODO to add this to the c'tor - better to reduce the dependency on flags for configuration inside ExecEnv.
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS2, Line 62: TBackendDescriptor
Comment on lifetime of pointer.
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
PS2, Line 95: TNetworkAddress krpc_svc_addr = MakeNetworkAddress(ip, FLAGS_krpc_port);
can't you get this from ExecEnv::GetInstance instead?
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
Line 323: /// backend or the local host itself.
comment on lifetime of return value.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
PS3, Line 39: Kudu RPC
that seems confusing since it has nothing to do with Kudu from the users' perspective. maybe it should say "KRPC-based ImpalaInternalService is exported"? or will this port be used outside of that?
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS3, Line 60: hostname
if it's a host name, why is the type TNetworkAddress as opposed to Hostname (like in LookupBackendIp())?
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
PS3, Line 95: FLAGS_krpc_port
do we have a check anywhere that this flag is set, and give an error message if not?
Line 96: local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr);
why does that have to get saved in the backend descriptor? since the port is constant and we already have the ip, why not just use those fields rather than creating another?
Line 241: DCHECK(desc != nullptr);
isn't this dcheck really saying that if LookupBackendDesc() returned nullptr, then we better have host == local_backend_descriptor_.address? If so, seems more direct to just DCHECK that instead of make it an if-stmt.
Line 711: // between ComputeScanRangeAssignment() and ComputeFragmentExecParams().
I don't understand that. isn't this just copying the shared_ptr, not the BackendConfig itself? so how does it avoid the inconsistency?
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
Line 1660: MakeNetworkAddress(ip, FLAGS_krpc_port));
another reason to not add krpc_svc_address but instead just generate it from ip and the krpc_port on the fly.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0);
> I am actually quite confused as to the guideline for using FLAGS in ExeEnv.
I don't think there's a hard and fast rule. In general I think it's not a bad idea to make configuration dependencies explicit, particularly in the case of ports where there's the possibility of conflict with the rest of the system. There's obviously a balance between making the c'tor too unwieldy and allowing some flexibility.
Here, I'd just ask that we treat the krpc_port the same as all other ports. Consistency is important.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/5/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:
PS5, Line 53: ImpalaInternal
> let's say "Impala internal service" just like you did for krpc_address to m
Done
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0);
> Add a TODO to add this to the c'tor - better to reduce the dependency on fl
I am actually quite confused as to the guideline for using FLAGS in ExeEnv. We seem to use them quite extensively here and other functions. It'd be great if you can clarify the guideline.
A previous version of this patch actually passes FLAGS_krpc_port to c'tor but it seems a bit redundant as it's actually not used.
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS2, Line 62: TBackendDescriptor
> Comment on lifetime of pointer.
Will do.
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
PS2, Line 95: TNetworkAddress krpc_svc_addr = MakeNetworkAddress(ip, FLAGS_krpc_port);
> can't you get this from ExecEnv::GetInstance instead?
It's actually not part of the ExecEnv. I guess it can be but we have to wait till ExecEnv::StartServices() to resolve the hostname to an IP address so may need to do it first thing in ExecEnv::StartServices() before calling Scheduler->Init().
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
Line 323: /// backend or the local host itself.
> comment on lifetime of return value.
Will do.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7760
to look at the new patch set (#3).
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch also takes into account of a problem found in
IMPALA-5795. In particular, the backend descriptor of the
coordinator may not be found in the backend map in the
scheduler if coordinator is not an executor (i.e. dedicated
coordinator). The fix to also check against the local backend
descriptor.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
13 files changed, 104 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/3
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7760
to look at the new patch set (#4).
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch also takes into account of a problem found in
IMPALA-5795. In particular, the backend descriptor of the
coordinator may not be found in the backend map in the
scheduler if coordinator is not an executor (i.e. dedicated
coordinator). The fix to also check against the local backend
descriptor.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
16 files changed, 149 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/4
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0);
> I think it would be better to keep things as they are. There aren't that ma
If the plan is to use ExecEnv in the long run, we need to clean up those places which access FLAGS_*_port instead.
Instead of a TODO, I may as well do it in this change.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7760/1//COMMIT_MSG
Commit Message:
Line 16:
It would also be good to mention that this contains the fix for the issues found while investigating IMPALA-5795, and what those fixes are.
http://gerrit.cloudera.org:8080/#/c/7760/1/be/src/scheduling/backend-config.cc
File be/src/scheduling/backend-config.cc:
PS1, Line 93: GetBackendListForHost
Would we not DCHECK inside here for if 'host' is the local_backend_descriptor_'s network address?
Or would we not find it inside the LookupBackendIp() call too?
http://gerrit.cloudera.org:8080/#/c/7760/1/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS1, Line 61: NULL
nit: nullptr
http://gerrit.cloudera.org:8080/#/c/7760/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 710: ExecutorsConfigPtr config_ptr = GetExecutorsConfig();
I think it would be good to add a comment explaining why you pass the 'config_ptr' here instead of retrieving it inside ComputeScanRangeAssignment() itself.
My guess is that we want a consistent view of the backends across the calls to ComputeScanRangeAssignment() and ComputeFragmentExecParams(), since the view of the backends can potentially change in between.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
PS4, Line 39: Kudu
still says Kudu. Isn't that confusing since it has nothing to do with kudu (other than being derived from it)? Maybe just call it "KRPC".
Or should we make this flag hidden so users don't find it until the feature is finished?
http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 72: // doesn't have to resolve it on every heartbeat.
would help to update that comment to include KRPC needing resolved IP.
PS4, Line 231: DCHECK
nit: DCHECK_EQ
http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
Line 353: // Initiating coordinator.
would be nice to indicate this is the thrift address.
Line 407: // ... which is being executed on this server
is that the thrift address? would be nice to comment that.
http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:
Line 53: // Network address of the Impala service on this backend
specify that's the thrift address
PS4, Line 73: svc_
maybe just call it krpc_address for consistency e.g. address/krpc_address and server/krpc_server naming. the 'address' field is also that of a "service". okay to ignore if you feel this naming is better.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 6: Code-Review+1
Rebase. Carry +1.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
PS3, Line 39: Kudu RPC
> that seems confusing since it has nothing to do with Kudu from the users' p
Done
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS3, Line 60: hostname
> if it's a host name, why is the type TNetworkAddress as opposed to Hostname
This is needed to compare the port too. Apparently, each hostname maps to a list of backend descriptor. We return the first descriptor with matching IP and port.
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
PS3, Line 95:
> do we have a check anywhere that this flag is set, and give an error messag
It's set to default value 29000 if the user doesn't specify it during startup. We can potentially do a check for the validity of its value but I don't see the need to make an exception for it if we aren't already testing the values of other ports or whether the port is already in use.
Line 241: ADD_TIMER(schedule->summary_profile(), "ComputeScanRangeAssignmentTimer");
> isn't this dcheck really saying that if LookupBackendDesc() returned nullpt
Done. Note that in release builds, we will just return the wrong descriptor. Is it better to crash or should we trust our testing to have covered it ?
I suppose similar questions can be asked about other DCHECKs in the existing code base.
Line 711: #endif
> I don't understand that. isn't this just copying the shared_ptr, not the Ba
The way the update happens is that it overwrites the shared_ptr "executors_config_" atomically to point to the new copy. So, if you call GetExecutorsConfig() twice, you can potentially get back two different pointers. Checking out a copy here (with shared_ptr) makes sure that we retain reference to the old copy even after "executors_config_" has been overwritten.
http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
Line 410: // IP address + port of the KRPC based services on the destination
> does this have to be resolved IP as well?
Done
http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:
Line 72: // IP address + port of KRPC based services on this backend
> this one has to be a resolved IP, right? let's make that clear.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:
Line 53: // Network address of the Impala service on this backend
> specify that's the thrift address
Done
PS4, Line 73: svc_
> maybe just call it krpc_address for consistency e.g. address/krpc_address a
Done
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
PS4, Line 39: Kudu
> still says Kudu. Isn't that confusing since it has nothing to do with kudu
OK. Renamed to KRPC to avoid confusion. I guess people may still ask what K stands for in KRPC at some point. Also hiding this flag for now.
http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 72: // doesn't have to resolve it on every heartbeat.
> would help to update that comment to include KRPC needing resolved IP.
Done
PS4, Line 231: DCHECK
> nit: DCHECK_EQ
Done
http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
Line 353: // Initiating coordinator.
> would be nice to indicate this is the thrift address.
Done
Line 407: // ... which is being executed on this server
> is that the thrift address? would be nice to comment that.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 96: local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr);
> Is your question why it has to be saved to the local backend descriptor? Or
Oh, I was confused by all the different places and ways we set the port and how we were doing that for thrift since it gets carried around differently. The more we can make things consistent, and have less redundant state too (i.e. both flags and ExecEnv fields), the better.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1181/
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
Line 1660: MakeNetworkAddress(ip, FLAGS_krpc_port));
> Either way seems fine to me but we still need to propagate the port number
No, let's not put the port in here separately. Leaving krpc_svc_address is okay with me.
http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
Line 410: // the address of the KRPC services on the destination
does this have to be resolved IP as well?
http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:
Line 72: // Network address of KRPC services on this backend
this one has to be a resolved IP, right? let's make that clear.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 7: Code-Review+2
Carry +2
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/5/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:
PS5, Line 53: Impala service
let's say "Impala internal service" just like you did for krpc_address to make it clear it's the same services. Or you could say ImpalaInternalService in both places, like you did in the other thrift file.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7760/1//COMMIT_MSG
Commit Message:
Line 16:
> It would also be good to mention that this contains the fix for the issues
Done
http://gerrit.cloudera.org:8080/#/c/7760/1/be/src/scheduling/backend-config.cc
File be/src/scheduling/backend-config.cc:
PS1, Line 93: GetBackendListForHost
> Would we not DCHECK inside here for if 'host' is the local_backend_descript
It should find it in LookupBackendIp(). See line 77 above.
http://gerrit.cloudera.org:8080/#/c/7760/1/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS1, Line 61: NULL
> nit: nullptr
Done
http://gerrit.cloudera.org:8080/#/c/7760/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 710: ExecutorsConfigPtr config_ptr = GetExecutorsConfig();
> I think it would be good to add a comment explaining why you pass the 'conf
Yes. We need to check out a copy upfront to avoid using different executor config between ComputeScanRangeAssignment() and ComputeFragmentExecParams().
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3: Code-Review+1
Carry +1
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0);
> As far as I understand, the only other user of this constructor (other than
I think it would be better to keep things as they are. There aren't that many FLAGS_*_port accesses - the main ones I see are from catalog-op-executor.cc and they could be replaced with a call to ExecEnv::catalog_service_port(). That's the point of the ExecEnv, as I understand it. I would rather we eventually remove the default c'tor of ExecEnv, honestly.
Anyhow, I'm just suggesting you add a TODO; we don't need to make a decision right now.
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
Line 96: local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr);
> why does that have to get saved in the backend descriptor? since the port i
Is your question why it has to be saved to the local backend descriptor? Or why any backend descriptor at all? For the former, it seems easier to manage all nodes uniformly. For the latter, the port may not be constant across the cluster (think about the minicluster scenario, for example).
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
IMPALA-4856: Include KRPC services in plan fragment's destinations
This change allows Impala to publish the address and port
information of KRPC services if it's enabled via the flag
use_krpc. The information is included in a new field in the
backend descriptor published as statestore updates. Scheduler
will also include this information in the destinations of plan
fragments. Also updated the mini-cluster startup script to assign
KRPC ports to Impalad instances.
This patch also takes into account of a problem found in
IMPALA-5795. In particular, the backend descriptor of the
coordinator may not be found in the backend map in the
scheduler if coordinator is not an executor (i.e. dedicated
coordinator). The fix to also check against the local backend
descriptor.
This patch is partially based on an abandoned patch by Henry Robinson.
Testing done: ran core tests with a patch which ignores the use_krpc
flag to exercise the code in scheduler.
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/network-util.cc
M be/src/util/network-util.h
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
13 files changed, 102 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/2
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:
PS2, Line 62: TBackendDescriptor
> Will do.
Done
http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:
Line 323: /// backend or the local host itself.
> Will do.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Add KRPC service address to backend descriptor and plan fragment's destination
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7760
to look at the new patch set (#3).
Change subject: IMPALA-4856: Add KRPC service address to backend descriptor and plan fragment's destination
......................................................................
IMPALA-4856: Add KRPC service address to backend descriptor and plan fragment's destination
Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
---
M be/src/common/global-flags.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/backend-config.cc
M be/src/scheduling/backend-config.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/network-util.cc
M be/src/util/network-util.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
12 files changed, 83 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/3
--
To view, visit http://gerrit.cloudera.org:8080/7760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>