You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2019/01/23 18:12:08 UTC
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12260
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the imapald can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 219 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/1
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
Further improvements to DoRpcWithRetry() will be done in IMPALA-8143.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 229 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/6
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1896/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Sat, 26 Jan 2019 02:28:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 221 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/3
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3743/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 08 Feb 2019 22:03:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
Further improvements to DoRpcWithRetry() will be done in IMPALA-8143.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 235 insertions(+), 150 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/7
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 7: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@695
PS6, Line 695: This may be because the port specified is a thrift port, which"
: " :shutdown() can no longer use.
> I'm not sure what you mean here. Yes we can't be sure that the reason we ca
"Using thrift port" is a subset of "using the wrong port". The case of "using thrift port" is true iff port == FLAGS_be_port which apparently isn't part of the if-stmt here so it's not precise to say so in the error message.
Anyhow, it's just a minor point so it's not worth dwelling on it. Please feel free to ignore the suggestion.
http://gerrit.cloudera.org:8080/#/c/12260/7/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/7/be/src/service/client-request-state.cc@691
PS7, Line 691: request.__isset.backend && request.backend.port
nit: factor this out common expression as it's used on line 639 above
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Thu, 07 Feb 2019 02:35:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 5:
(4 comments)
Thanks Michael for comments. I think in general I would prefer to finish this change and do some followup work in IMPALA-8143. Or I can do that work as part of this change, what do you think?
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@84
PS4, Line 84: ic Status DoRpcWithRet
> Instead of passing the MonoDelta object itself, how about just defining thi
My experience in the Java world with TimeUnit is that using a type like MonoDelta makes code more readable and avoids mistakes.
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@85
PS4, Line 85: const char* debug_actio
> DCHECK_GT()
Done
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@92
PS4, Line 92: // Check for injected failures.
> Please add a TODO about sleeping between retries in case the server is over
I added the TODO, thanks. I didn't add the actual sleep as that was what we agreed in our offline meeting, I though we said we would do this work in a followup jira, which I added: IMPALA-8143. I agree that the code is fairly simple... but there are unresolved questions: how long should the sleep be? Do we want exponential backoff? How can we test this?
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170
PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES);
> This can easily be replaced by DebugAction. Please see https://github.com/a
OK, how about I promise to do this in IMPALA-8143? I can see some other problems with test_rpc_timeout.py that need to be fixed and I will need to add some sort of test for the new timeout.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 01 Feb 2019 18:27:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 6:
(10 comments)
Thanks Michael
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@692
PS6, Line 692: msg.find("RemoteShutdown() RPC failed: Timed out: connection negotiation to")
: != string::npos
> Do you think it makes sense to spill out this message iff "request.__isset.
Done
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@695
PS6, Line 695: This may be because the port specified is a thrift port, which"
: " :shutdown() can no longer use.
> Will it be more precise to say as we didn't match the port specified (if an
I'm not sure what you mean here. Yes we can't be sure that the reason we can't connect is because the user specified the thrift port. On the other hand, specifying the thrift port used to work, so it is possible that that is what is happening. I'm trying to help prevent calls to support because we changed some behavior that used to work.
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h@71
PS6, Line 71: class
> ?
Done
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h@72
PS6, Line 72: class
> ?
Done
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@79
PS4, Line 79: /// Retry the Rpc 'rpc_call' up to 'times_to_try' times.
> May also want to mention each RPC has a timeout of 'timeout' in some time u
Done
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@84
PS4, Line 84: ic Status DoRpcWithRet
> Fair point.
Done
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@92
PS4, Line 92: // Check for injected failures.
> Sure, please address those questions in IMPALA-8143.
Done
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170
PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES);
> Please add a TODO IMPALA-8143 as a reminder.
Done
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@184
PS3, Line 184: FAULT_INJECTION_RPC_DELAY(RPC_REMOTESHUTDOWN);
> These tests are setting delays on server-side commands so as to test RPC ti
Done
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.cc@98
PS6, Line 98: kudu::rpc::RpcContext
> Not your change but can you please replace "kudu::rpc::RpcContext" to RpcCo
Done
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 06 Feb 2019 18:15:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h@70
PS1, Line 70: virtual void RemoteShutdown(const class RemoteShutdownParamsPB* req,
Brief comment
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h@81
PS1, Line 81: Rrpc
typo?
I guess it was already this way, I just missed it in the last review.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 25 Jan 2019 18:23:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1918/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 30 Jan 2019 00:19:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@79
PS2, Line 79: /// Retry the Rpc 'rpc_call' up to 3 times.
: /// Pass 'debug_action' to DebugAction() to potentially inject errors.
: template <typename F>
: static Status DoRpcWithRetry(F&& rpc_call, const TQueryCtx& query_ctx,
: const char* debug_action, const char* error_msg) {
> I am fine with doing the refactoring with a separate JIRA as long as it doe
Since the retry loop in query-state is getting significantly rewritten in my patch for IMPALA-4555 anyways, I would suggest not touching that in this patch
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 29 Jan 2019 20:30:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
Further improvements to DoRpcWithRetry() will be done in IMPALA-8143.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 229 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/5
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 2: Code-Review+1
lgtm, but I'll give Michael a chance to do a pass if he wants
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Mon, 28 Jan 2019 22:40:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@653
PS2, Line 653: // KRPC relies on resolved IP address, so convert hostname.
: IpAddr ip_address;
: Status ip_status = HostnameToIpAddr(request.backend.hostname, &ip_address);
: if (!ip_status.ok()) {
: VLOG(1) << "Could not convert hostname " << request.backend.hostname
: << " to ip address, error: " << ip_status.GetDetail();
: return ip_status;
: }
: TNetworkAddress addr = MakeNetworkAddress(ip_address, port);
> In my original (unpublished) prototype I was keeping the original UX where
Thanks for the explanation. That's a reasonable scenario in which the backend being shut down may not be found in the backend descriptors.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@692
PS2, Line 692: return Status(Substitute("Rpc to $0 failed with error '$1'. This may be because "
: "the port specified is a thrift port, which :shutdown() "
: "can no longer use. Make sure the port is a KRPC port.",
: TNetworkAddressToString(addr), msg));
: }
: return Status(Substitute(
: "Rpc to $0 failed with error '$1'.", TNetworkAddressToString(addr), msg));
Not sure if a typical user has any understanding of the difference between Thrift port and KPRC port as they are internal to Impala. May we can simplify the statement to be like the following:
string err_string = Substitute("Rpc to $0 failed with error '$1'",
TNetworkAddressToString(addr), msg);
if (msg.find(...) != string::npos) {
err_string.append(" Please make sure the right port is being used or don't specify any port in the :shutdown() command");
}
return Status(err_string);}
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170
PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES);
Shouldn't this use a DebugAction instead ?
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@184
PS3, Line 184: FAULT_INJECTION_RPC_DELAY(RPC_REMOTESHUTDOWN);
Shouldn't this use a DebugAction instead ?
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 29 Jan 2019 23:11:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 6:
Patch set 6 is just to fix merge conflicts.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 05 Feb 2019 17:54:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 1:
(2 comments)
Thanks for comments
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h@70
PS1, Line 70: virtual void RemoteShutdown(const class RemoteShutdownParamsPB* req,
> Brief comment
Done
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h@81
PS1, Line 81: Rrpc
> typo?
I had to stare to see this, thanks
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Sat, 26 Jan 2019 00:54:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@79
PS2, Line 79: /// Retry the Rpc 'rpc_call' up to 3 times.
: /// Pass 'debug_action' to DebugAction() to potentially inject errors.
: template <typename F>
: static Status DoRpcWithRetry(F&& rpc_call, const TQueryCtx& query_ctx,
: const char* debug_action, const char* error_msg) {
> Thanks. The idea (suggested by Thomas) was for DoRpcWithRetry to be reusabl
I am fine with doing the refactoring with a separate JIRA as long as it doesn't become a back burner somehow. That said, it seems to make sense to add a sleep time between retries in this patch at least for the case of remote server being too busy (see the example in KrpcDataStreamSender).
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@87
PS2, Line 87: MonoDelta::FromSeconds(10)
> Yes, I wondered about this too. I decided that while my 2 clients are using
Given what you stated above about DoRpcWithRetry() being reusable, I am not sure it makes sense to make assumption about the clients and hardcode the timeout / number of retries / sleep time between retries.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2489
PS2, Line 2489: ShutdownStatusPB
> Is that safe? GetShutdownStatus() allocates the ShutdownStatusPB on the sta
It is an official C++ feature to extend the life time of a temporary object to the life time of the const reference which refers to it.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 29 Jan 2019 19:12:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1859/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 23 Jan 2019 18:38:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 08 Feb 2019 22:02:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12260/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12260/1//COMMIT_MSG@13
PS1, Line 13: imapald
typo
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@653
PS2, Line 653: // KRPC relies on resolved IP address, so convert hostname.
: IpAddr ip_address;
: Status ip_status = HostnameToIpAddr(request.backend.hostname, &ip_address);
: if (!ip_status.ok()) {
: VLOG(1) << "Could not convert hostname " << request.backend.hostname
: << " to ip address, error: " << ip_status.GetDetail();
: return ip_status;
: }
: TNetworkAddress addr = MakeNetworkAddress(ip_address, port);
Instead of resolving the IP for the backend, it seems more consistent to re-use some of the logic at the scheduler which already contains a map of all the backend state:
https://github.com/apache/impala/blob/master/be/src/scheduling/scheduler.cc#L354-L357
May need some refactoring in the scheduler code to expose the necessary backend states. It's unfortunate that the existing 'executor_config_' uses 'hostname, be_port' as lookup key. May be it makes sense to implement a new function which looks up if there exists a known backend with 'hostname and krpc_address which exposes the port specified' and if so, returns the krpc_address_. If there isn't a known backend which maps to the specified 'host + krpc port', then we should just fail the shutdown request without making the actual RPC.
Please feel free to come up with other ideas too.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@689
PS2, Line 689: if (msg.find("RemoteShutdown() RPC failed: Timed out: connection negotiation to")
: != string::npos) {
If we take the route of using existing backend states suggested above, the case of invalid port should be impossible so we shouldn't need to call this out like below here.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@79
PS2, Line 79: /// Retry the Rpc 'rpc_call' up to 3 times.
: /// Pass 'debug_action' to DebugAction() to potentially inject errors.
: template <typename F>
: static Status DoRpcWithRetry(F&& rpc_call, const TQueryCtx& query_ctx,
: const char* debug_action, const char* error_msg) {
Not specifically related to this change but it may be useful to factor this retry function to generic RPC code in rpc-mgr.inline.h.
Also, if the RPC fails due to being overloaded, it may make sense to have some sleep time between retries.
FWIW, we had some retry logic in KrpcDataStreamSender too and I believe we also had similar retry logic for the status reporting code. It may be worth the effort to consider refactoring these code so we don't implement the same retry logic at multiple places:
https://github.com/apache/impala/blob/master/be/src/runtime/krpc-data-stream-sender.cc#L374-L386
https://github.com/apache/impala/blob/master/be/src/runtime/query-state.cc#L332-L336
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@87
PS2, Line 87: MonoDelta::FromSeconds(10)
Not sure if it makes sense to hard code the timeout here as it seems to be utility function used by different RPC calls. Would it be better for the timeout to be one of the function parameter ? At the minimum, if we decide that 10 seconds is the timeout for all RPCs in ControlService, we should at least define it as some constant or #define to make it more explicit.
I have a similar question about the number of retries too (which is hardcoded to 3).
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h@81
PS1, Line 81: name F>
typo ? DoRpcWithRetry ?
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.cc@182
PS2, Line 182: class
Why is this needed ?
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.cc@183
PS2, Line 183: class
Why is this needed ?
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2435
PS2, Line 2435: "queries registered on coordinator: $2, queries executing: $3, "
The original formatting seems fine to me.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2481
PS2, Line 2481: if (set_deadline)
: shutdown_status->set_deadline_remaining_ms(relative_deadline_s * 1000L);
Coding style nit:
if (set_deadline) {
....
}
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2489
PS2, Line 2489: ShutdownStatusPB
const ShutdownStatusPB&
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 29 Jan 2019 07:56:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1990/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 05 Feb 2019 18:27:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/runtime/coordinator-backend-state.cc@445
PS6, Line 445: 10.0L
10 should be sufficient. It will be implicitly casted to double.
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@692
PS6, Line 692: msg.find("RemoteShutdown() RPC failed: Timed out: connection negotiation to")
: != string::npos
Do you think it makes sense to spill out this message iff "request.__isset.backend && request.backend.port != 0" ?
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/client-request-state.cc@695
PS6, Line 695: This may be because the port specified is a thrift port, which"
: " :shutdown() can no longer use.
Will it be more precise to say as we didn't match the port specified (if any) is actually a thrift port number, right ?
"This may be because the port specified is wrong."
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h@71
PS6, Line 71: class
?
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.h@72
PS6, Line 72: class
?
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@84
PS4, Line 84: MonoDelta krpc_timeout
> My experience in the Java world with TimeUnit is that using a type like Mon
Fair point.
The coding convention in Impala code (e.g. see StartShutdown()) tends to be that we specific time with a suffix of the time unit instead of passing MonoDelta object around. It seems more consistent with the existing code to use timeout_ms or timeout_s instead.
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@92
PS4, Line 92: if (!rpc_status.ok()) continue;
> I added the TODO, thanks. I didn't add the actual sleep as that was what we
Sure, please address those questions in IMPALA-8143.
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170
PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES);
> OK, how about I promise to do this in IMPALA-8143? I can see some other pro
Please add a TODO IMPALA-8143 as a reminder.
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/6/be/src/service/control-service.cc@98
PS6, Line 98: kudu::rpc::RpcContext
Not your change but can you please replace "kudu::rpc::RpcContext" to RpcContext given the "use kudu::rpc::RpcContext" above ?
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 05 Feb 2019 19:40:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
Further improvements to DoRpcWithRetry() will be done in IMPALA-8143.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 226 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/4
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@692
PS2, Line 692: if (msg.find("RemoteShutdown() RPC failed: Timed out: connection negotiation to")
: != string::npos) {
: // Prior to IMPALA-7985 :shutdown() used the backend port.
: err_string.append(" This may be because the port specified is a thrift port, which"
: " :shutdown() can no longer use. Please make sure the right port"
: " is being used, or don't specify any port in the :shutdown()"
: " command.");
> Not sure if a typical user has any understanding of the difference between
Done
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170
PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES);
> Shouldn't this use a DebugAction instead ?
These tests are setting delays on server-side commands so as to test RPC timeouts. They FAULT_INJECTION_RPC_DELAY injections are configured by running servers with --fault_injection_rpc_type=XXX.
DebugAction on the other hand is configured for a particular query by passing --query_option=YYY to a client like impala-shell.
So in this case FAULT_INJECTION_RPC_DELAY seems the right choice.
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@184
PS3, Line 184: FAULT_INJECTION_RPC_DELAY(RPC_REMOTESHUTDOWN);
> Shouldn't this use a DebugAction instead ?
These tests are setting delays on server-side commands so as to test RPC timeouts. They FAULT_INJECTION_RPC_DELAY injections are configured by running servers with --fault_injection_rpc_type=XXX.
DebugAction on the other hand is configured for a particular query by passing --query_option=YYY to a client like impala-shell.
So in this case FAULT_INJECTION_RPC_DELAY seems the right choice.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2489
PS2, Line 2489: SleepForMs(1000)
> It is an official C++ feature to extend the life time of a temporary object
Thanks, done
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 30 Jan 2019 20:01:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2001/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 06 Feb 2019 18:58:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
Further improvements to DoRpcWithRetry() will be done in IMPALA-8143.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Enhance the test for /backends in test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 231 insertions(+), 152 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/8
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2046/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 08 Feb 2019 22:34:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1961/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 01 Feb 2019 19:10:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 3:
(10 comments)
Thanks Michael for comments
http://gerrit.cloudera.org:8080/#/c/12260/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12260/1//COMMIT_MSG@13
PS1, Line 13: impalad
> typo
Done
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/client-request-state.cc@653
PS2, Line 653: // KRPC relies on resolved IP address, so convert hostname.
: IpAddr ip_address;
: Status ip_status = HostnameToIpAddr(request.backend.hostname, &ip_address);
: if (!ip_status.ok()) {
: VLOG(1) << "Could not convert hostname " << request.backend.hostname
: << " to ip address, error: " << ip_status.GetDetail();
: return ip_status;
: }
: TNetworkAddress addr = MakeNetworkAddress(ip_address, port);
> Instead of resolving the IP for the backend, it seems more consistent to re
In my original (unpublished) prototype I was keeping the original UX where the user specified the backed thrift port. I then exposed LookUpBackendDesc and used it to find the thrift port of the impalad. As discussed in the Jira, the trouble with this is that it relies on the current cluster membership being correct. The corollary is that the user can't shutdown a failing remote impalad that has left the cluster. So I'm not sure that it is best to rely on the Scheduler's knowledge of the world here.
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@79
PS2, Line 79: /// Retry the Rpc 'rpc_call' up to 3 times.
: /// Pass 'debug_action' to DebugAction() to potentially inject errors.
: template <typename F>
: static Status DoRpcWithRetry(F&& rpc_call, const TQueryCtx& query_ctx,
: const char* debug_action, const char* error_msg) {
> Not specifically related to this change but it may be useful to factor this
Thanks. The idea (suggested by Thomas) was for DoRpcWithRetry to be reusable. The KrpcDataStreamSender code looks very specific to its class, but the QueryState code may be able to use DoRpcWithRetry. The flexibility of DoRpcWithRetry is that it uses a lambda, but that may make that QueryState sidecar code tricky as the fun part of the lambda is the capture clause. Maybe I should look at this in a separate jira?
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.h@87
PS2, Line 87: MonoDelta::FromSeconds(10)
> Not sure if it makes sense to hard code the timeout here as it seems to be
Yes, I wondered about this too. I decided that while my 2 clients are using the same numbers that I wouldn't expose these as parameters, but it is easy to do if we want.
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/1/be/src/service/control-service.h@81
PS1, Line 81: name F>
> typo ? DoRpcWithRetry ?
Done
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.cc@182
PS2, Line 182: Remot
> Why is this needed ?
It's not, I removed it
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/control-service.cc@183
PS2, Line 183: Remot
> Why is this needed ?
Done
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2435
PS2, Line 2435: "queries registered on coordinator: $2, queries executing: $3, "
> The original formatting seems fine to me.
I'm must doing what clang-format wants :-)
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2481
PS2, Line 2481: if (set_deadline) {
: shutdown_status->set_deadline_remaining_ms(relative_deadline_s * 1000L);
> Coding style nit:
Done
http://gerrit.cloudera.org:8080/#/c/12260/2/be/src/service/impala-server.cc@2489
PS2, Line 2489: SleepForMs(1000)
> const ShutdownStatusPB&
Is that safe? GetShutdownStatus() allocates the ShutdownStatusPB on the stack so I think I want to return it by value.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 29 Jan 2019 18:52:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1937/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 30 Jan 2019 20:44:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@79
PS4, Line 79: /// Retry the Rpc 'rpc_call' up to 'times_to_try' times.
May also want to mention each RPC has a timeout of 'timeout' in some time units.
Also, there is no sleeping between retries.
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@84
PS4, Line 84: MonoDelta krpc_timeout
Instead of passing the MonoDelta object itself, how about just defining this as timeout in millisecond ?
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@85
PS4, Line 85: DCHECK(times_to_try > 0);
DCHECK_GT()
http://gerrit.cloudera.org:8080/#/c/12260/4/be/src/service/control-service.h@92
PS4, Line 92: if (!rpc_status.ok()) continue;
Please add a TODO about sleeping between retries in case the server is overloaded. Honestly though, I don't see why we cannot add a sleep time between retries in this patch if the remote server's service queue fills up. Otherwise, the retry loop won't be too effective as we burst sending the same RPCs in such a short period of time.
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@170
PS3, Line 170: FAULT_INJECTION_RPC_DELAY(RPC_CANCELQUERYFINSTANCES);
> These tests are setting delays on server-side commands so as to test RPC ti
This can easily be replaced by DebugAction. Please see https://github.com/apache/impala/blob/master/tests/custom_cluster/test_rpc_timeout.py#L145-L150
In general, we should converge on using DebugAction for any type of fault injection instead of building on the old infrastructure.
http://gerrit.cloudera.org:8080/#/c/12260/3/be/src/service/control-service.cc@184
PS3, Line 184: FAULT_INJECTION_RPC_DELAY(RPC_REMOTESHUTDOWN);
> These tests are setting delays on server-side commands so as to test RPC ti
Please see replies above.
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Thu, 31 Jan 2019 19:00:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Fri, 08 Feb 2019 22:03:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the imapald can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Add a test for /backends to test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 220 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12260/2
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
IMPALA-7985: Port RemoteShutdown() to KRPC.
The :shutdown command is used to shutdown a remote server. The common
case is that a user specifies the impalad to shutdown by specifying a
host e.g. :shutdown('host100'). If a user has more than one impalad on a
remote host then the form :shutdown('<host>:<port>') can be used to
specify the port by which the impalad can be contacted. Prior to
IMPALA-7985 this port was the backend port, e.g.
:shutdown('host100:22000'). With IMPALA-7985 the port to use is the KRPC
port, e.g. :shutdown('host100:27000').
Shutdown is implemented by making an rpc call to the target impalad.
This changes the implementation of this call to use KRPC.
To aid the user in finding the KRPC port, the KRPC address is added to
the /backends section of the debug web page.
We attempt to detect the case where :shutdown is pointed at a thrift
port (like the backend port) and print an informative message.
Documentation of this change will be done in IMPALA-8098.
Further improvements to DoRpcWithRetry() will be done in IMPALA-8143.
For discussion of why it was chosen to implement this change in an
incompatible way, see comments in
https://issues.apache.org/jira/browse/IMPALA-7985.
TESTING
Ran all end-to-end tests.
Enhance the test for /backends in test_web_pages.py.
In test_restart_services.py add a call to the old backend port to the
test. Some expected error messages were changed in line with what KRPC
returns.
Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Reviewed-on: http://gerrit.cloudera.org:8080/12260
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/backend-client.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/service/client-request-state.cc
M be/src/service/control-service.cc
M be/src/service/control-service.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/protobuf/control_service.proto
M common/thrift/ImpalaInternalService.thrift
M tests/custom_cluster/test_restart_services.py
M tests/webserver/test_web_pages.py
M www/backends.tmpl
16 files changed, 231 insertions(+), 152 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7985: Port RemoteShutdown() to KRPC.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12260 )
Change subject: IMPALA-7985: Port RemoteShutdown() to KRPC.
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fd00ee4e638f5e71e27893162fd65501ef9e74e
Gerrit-Change-Number: 12260
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Sat, 09 Feb 2019 02:18:07 +0000
Gerrit-HasComments: No