[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/10835 ) Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/10835/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10835/3//COMMIT_MSG@7 PS3, Line 7: Deprecate Nit: I think it would be clearer to say "Remove" rather than "Deprecate". http://gerrit.cloudera.org:8080/#/c/10835/3//COMMIT_MSG@9 PS3, Line 9: deprecates Same here. http://gerrit.cloudera.org:8080/#/c/10835/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/10835/3/be/src/common/global-flags.cc@260 PS3, Line 260: REMOVED_FLAG(disable_mem_pools); What are our rules about removing startup flags? Should use_krpc be here? (See IMPALA-3271 and IMPALA-5814) -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 13 Jul 2018 18:48:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10835 ) Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. Patch Set 3: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/10835/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/10835/3/be/src/common/global-flags.cc@40 PS3, Line 40: DEFINE_int32(krpc_port, 27000, : "port on which KRPC based ImpalaInternalService is exported"); Now that we're exposing this, we should probably document this (if we're not already doing so). http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py@a73 PS1, Line 73: > ReportExecStatus() can be very timing sensitive as the number of iterations So that goes to say that we won't benefit much from running with this for ReportExecStatus() or CancelQueryFInstnaces() ? -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 12 Jul 2018 21:39:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10835 to look at the new patch set (#3). Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services This change deprecates the flag --use_krpc which allows users to fall back to using Thrift based implementation of DataStream services. This flag was originally added during development of IMPALA-2567. It has served its purpose. As we port more ImpalaInternalServices to use KRPC, it's becoming increasingly burdensome to maintain parallel implementation of the RPC handlers. Therefore, going forward, KRPC is always enabled. This change removes the Thrift based implemenation of DataStreamServices and also simplifies some of the tests which were skipped when KRPC is disabled. Testing done: core debug build. Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 --- M be/src/common/global-flags.cc M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/thrift-server-test.cc M be/src/runtime/CMakeLists.txt M be/src/runtime/backend-client.h D be/src/runtime/data-stream-mgr-base.h D be/src/runtime/data-stream-mgr.h D be/src/runtime/data-stream-recvr-base.h D be/src/runtime/data-stream-recvr.cc D be/src/runtime/data-stream-recvr.h D be/src/runtime/data-stream-sender.cc D be/src/runtime/data-stream-sender.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/krpc-data-stream-mgr.cc M be/src/runtime/krpc-data-stream-mgr.h M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/scheduling/scheduler.cc M be/src/service/data-stream-service.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 bin/run-all-tests.sh M bin/start-impala-cluster.py M common/thrift/ImpalaInternalService.thrift M tests/common/custom_cluster_test_suite.py M tests/common/skip.py D tests/common/test_skip.py M tests/conftest.py M tests/custom_cluster/test_krpc_mem_usage.py M tests/custom_cluster/test_krpc_metrics.py M tests/custom_cluster/test_rpc_exception.py M tests/query_test/test_codegen.py M tests/webserver/test_web_pages.py 44 files changed, 87 insertions(+), 2,142 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10835/3 -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/10835 )
Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old
DataStream services
..
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@23
PS1, Line 23: #include "common/status.h"
> I think I've found all the dead code in this patch, but if you want a refer
Thanks for checking.
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@128
PS1, Line 128: class DataStreamTest : public testing::Test {
: protected:
: DataStreamTest() : next_val_(0) {
: // Stop tests that rely on mismatched sender / receiver
pairs timing out from failing.
:
> No longer needed.
Done
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@134
PS1, Line 134: ~DataStreamTest() { runtime_state_->ReleaseResources(); }
:
: virtual void SetUp() {
:
> No longer needed.
Done
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@139
PS1, Line 139: erPool(32 * 1024, 1024 * 1024 * 1024, 32 * 1024);
> You can replace this with:
Done
http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:
http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py@a73
PS1, Line 73:
> Can't this happen without TransmitData() today? Don't we also call it on R
ReportExecStatus() can be very timing sensitive as the number of iterations
it's invoked depends on how long the query runs for.
--
To view, visit http://gerrit.cloudera.org:8080/10835
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3
Gerrit-Change-Number: 10835
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Wed, 11 Jul 2018 22:02:48 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/10835 )
Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old
DataStream services
..
Patch Set 1:
(5 comments)
LGTM overall. Just some comments around the test code.
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@23
PS1, Line 23: #include "common/status.h"
I think I've found all the dead code in this patch, but if you want a
reference, you can have a look at this patch to see if you need to remove
anything else:
https://github.com/apache/impala/commit/ff86feaa67ff8bf703896e33d9a358e42739ae30#diff-0b7021d2a8bfaf517b243f11a53bcde8
I added that when I was making this test KRPC compatible.
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@128
PS1, Line 128: template class DataStreamTestBase : public T {
: protected:
: virtual void SetUp() {}
: virtual void TearDown() {}
: };
No longer needed.
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@134
PS1, Line 134: enum KrpcSwitch {
: USE_THRIFT,
: USE_KRPC
: };
No longer needed.
http://gerrit.cloudera.org:8080/#/c/10835/1/be/src/runtime/data-stream-test.cc@139
PS1, Line 139: public DataStreamTestBase>
You can replace this with:
public testing::Test
http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:
http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py@a73
PS1, Line 73:
> We should probably look into removing certain injected exception if it's no
Can't this happen without TransmitData() today? Don't we also call it on
ReportExecStatus() and CancelQueryFInstances() ?
https://github.com/apache/impala/blob/fb8ea6a9ccaedc41a71f6f6dcb367fc1facd73b6/be/src/runtime/backend-client.h#L58-L76
--
To view, visit http://gerrit.cloudera.org:8080/10835
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3
Gerrit-Change-Number: 10835
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Tue, 03 Jul 2018 18:36:28 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10835 ) Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. Patch Set 1: Thanks for taking a look. Answers to your questions: 1. This is slated for Impala 3.1. We will just have to deal with problem in KRPC as it comes. I believe the biggest risk is the Kudu Kerberos implementation. We should have better test coverage for that area. It's burdensome and potentially error prone to have two implementations of the RPC handlers so at some point, we just bite the bullet and make the switch. 2. Yes, we should rename them but I refrain from doing so in this patch to make it easier to review. The renaming can be done in a follow-on patch as it's mostly mechanical. 3. Mostly mechanical. The only challenge may be removing Thrift-only tests which may benefit from some more scrutiny during review. -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 29 Jun 2018 20:45:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10835 ) Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. Patch Set 1: Some high level questions: 1. At what point do we want this checked in? 2.12.0 was the first release with KRPC and we've not gotten enough user feedback yet to know all the issues with the feature. Should we bite the bullet and deal with the issues as they come? Or should we leave Thrift RPC as a fallback for a while? 2. Should we rename all the KrpcDataStream* to DataStream* now since there's only one implementation now? Or should we keep a buffed period of releases before renaming to avoid confusion? 3. Was this patch mostly mechanical? Or did you face any issue(s) in getting rid of the flag and DataStream* classes? -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 29 Jun 2018 16:29:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10835 ) Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/10835/1/tests/custom_cluster/test_rpc_exception.py@a73 PS1, Line 73: We should probably look into removing certain injected exception if it's not going to be triggered easily without TransmitData() RPC. -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 27 Jun 2018 02:09:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7212: Deprecate --use krpc flag and remove old DataStream services
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10835 Change subject: IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services .. IMPALA-7212: Deprecate --use_krpc flag and remove old DataStream services This change deprecates the flag --use_krpc which allows users to fall back to using Thrift based implementation of DataStream services. This flag was originally added during development of IMPALA-2567. It has served its purpose. As we port more ImpalaInternalServices to use KRPC, it's becoming increasingly burdensome to maintain parallel implementation of the RPC handlers. Therefore, going forward, KRPC is always enabled. This change removes the Thrift based implemenation of DataStreamServices and also simplifies some of the tests which were skipped when KRPC is disabled. Testing done: core debug build. Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/thrift-server-test.cc M be/src/runtime/CMakeLists.txt M be/src/runtime/backend-client.h D be/src/runtime/data-stream-mgr-base.h D be/src/runtime/data-stream-mgr.h D be/src/runtime/data-stream-recvr-base.h D be/src/runtime/data-stream-recvr.cc D be/src/runtime/data-stream-recvr.h D be/src/runtime/data-stream-sender.cc D be/src/runtime/data-stream-sender.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/krpc-data-stream-mgr.cc M be/src/runtime/krpc-data-stream-mgr.h M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/scheduling/scheduler.cc M be/src/service/data-stream-service.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 bin/run-all-tests.sh M bin/start-impala-cluster.py M common/thrift/ImpalaInternalService.thrift M tests/common/custom_cluster_test_suite.py M tests/common/skip.py D tests/common/test_skip.py M tests/conftest.py M tests/custom_cluster/test_krpc_mem_usage.py M tests/custom_cluster/test_krpc_metrics.py M tests/custom_cluster/test_rpc_exception.py M tests/query_test/test_codegen.py M tests/webserver/test_web_pages.py 43 files changed, 85 insertions(+), 2,129 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10835/1 -- To view, visit http://gerrit.cloudera.org:8080/10835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icfed200751508478a3d728a917448f2dabfc67c3 Gerrit-Change-Number: 10835 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
