[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 9 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 22:27:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action IMPALA_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action IMPALA_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Reviewed-on: http://gerrit.cloudera.org:8080/14641 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 26 files changed, 242 insertions(+), 281 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 10 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5161/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 18:31:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 9 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 17:47:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5302/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 9 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 17:47:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 8: Code-Review+2 (1 comment) carrying forward http://gerrit.cloudera.org:8080/#/c/14641/7/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/7/be/src/util/debug-util.h@146 PS7, Line 146: const std::vector > const-reference Done -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 17:47:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14641 to look at the new patch set (#8). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action IMPALA_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action IMPALA_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 26 files changed, 242 insertions(+), 281 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/8 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 7 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 00:55:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14641/7/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/7/be/src/util/debug-util.h@146 PS7, Line 146: std::vector args) const-reference -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 00:55:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5138/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 26 Nov 2019 00:02:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@50 PS5, Line 50: ntity, > Please document this arg too. Done http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@115 PS5, Line 115: : /// The address th > These can be const, right ? Done http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@153 PS5, Line 153: WARN_UNUSED_RESULT > nit: Not your change, but can this be moved to the end of the declaration ? As far as I can tell, gcc considers that to be an error - it says "attributes are not allowed on a function-definition" http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@154 PS5, Line 154: const std::vector const std::vector& ? Done http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@159 PS5, Line 159: WARN_UNUSED_RESULT > nit: Not your change, but can this be moved to the end of the declaration ? Done http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@161 PS5, Line 161: > Given the default arg above, is this necessary or is it just for clarity ? Good point, removed. http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@165 PS5, Line 165: ; > Given the default arg above, is this necessary or is it just for clarity ? Done http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@92 PS5, Line 92: :...: > Given that ":" is already used for delimiting different component of the de I wouldn't say that ':' is being re-used for a different purpose here, I would say this patch just extends the existing purpose of ':' to new situations. Or in other words, for the ExecNode actions ':' is used to delimit a list of arguments that must match for the debug action to be executed, just like in this patch. Not a big deal, I can change it if you really find it confusing, but I think it makes more sense this way, and it keeps the parsing code simpler. http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@105 PS5, Line 105: //omitted, a generic error of the form: 'Debug Action: :' is used. > May help to briefly document the purpose of arg list. Done -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 25 Nov 2019 23:23:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14641 to look at the new patch set (#6). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action IMPALA_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action IMPALA_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 26 files changed, 242 insertions(+), 281 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/6 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@50 PS5, Line 50: address Please document this arg too. http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@115 PS5, Line 115: std::string hostname_; : std::string port_; These can be const, right ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@153 PS5, Line 153: WARN_UNUSED_RESULT nit: Not your change, but can this be moved to the end of the declaration ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@154 PS5, Line 154: std::vector const std::vector& ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@159 PS5, Line 159: WARN_UNUSED_RESULT nit: Not your change, but can this be moved to the end of the declaration ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@161 PS5, Line 161: std::vector() Given the default arg above, is this necessary or is it just for clarity ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@165 PS5, Line 165: std::vector() Given the default arg above, is this necessary or is it just for clarity ? http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@92 PS5, Line 92: :...: Given that ":" is already used for delimiting different component of the debug action, do you think it may be slightly clearer to not re-use ":" for separating the args ? Instead, can we use "," or other character ? http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@105 PS5, Line 105: //exercising different error paths. May help to briefly document the purpose of arg list. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 25 Nov 2019 21:31:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5017/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 13 Nov 2019 21:43:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: Code-Review+1 Carrying +1. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 13 Nov 2019 21:08:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14641 to look at the new patch set (#5). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action IMPALA_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action IMPALA_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 26 files changed, 239 insertions(+), 282 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/5 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 4: Code-Review+1 LGTM. Patch looks like it needs to be re-based. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 13 Nov 2019 20:54:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5016/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 13 Nov 2019 20:35:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14641 to look at the new patch set (#4). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action IMPALA_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action IMPALA_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 26 files changed, 238 insertions(+), 282 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/4 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Tauber-Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options
..
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/14641/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14641/3//COMMIT_MSG@26
PS3, Line 26: IMPALA_SERVICE_P
> IMPALA_SERVICE_POOL?
Done
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/impala-service-pool.cc@207
PS3, Line 207: FailAndReleaseRpc
> perhaps not in this patch, but would it be good to have an option to just c
For sure. Lots of potential for additional testing in this basic area.
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/rpc-mgr.cc@193
PS3, Line 193: // Convert 'address_' to Kudu's Sockaddr
> move this to Init? since that is when address is passed in?
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@395
PS2, Line 395: }
: string error_msg = tokens.size() == 3 ?
: tokens[2] :
> Yeah, didn't see the check for "iequals(cmd, "FAIL")" above.
Done
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/util/debug-util.cc@400
PS3, Line 400: if (ImpaladMetrics::DEBUG_ACTION_NUM_FAIL != nullptr) {
> this should never be nullptr right? add a DCHECK asserting it is never a nu
No - as currently written, this patch only creates this if FLAGS_debug_actions
is set, but its possible there could still be a debug_action set as a query
option instead.
I was thinking that if we want these metrics for tests that use the
debug_action query option, we could do something like extend TestInfo to
support is_ee_test() via a flag that gets passed in to start-impala-cluster.py
in run-all-tests.sh or something like that, I just hadn't done it because its
not required for this patch.
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@92
PS2, Line 92: ::...::@@@..."
> Yeah, maybe passing in a path to a file that contains JSON is the right way
Done
http://gerrit.cloudera.org:8080/#/c/14641/3/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/14641/3/common/thrift/ImpalaService.thrift@101
PS3, Line 101: ability>][
> should mention that the DebugAction might change code paths depending on th
Done
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@63
PS3, Line 63:
> is this used anywhere?
Done
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@63
PS3, Line 63: execute_test_query
> Not sure how easy this would be, more of a suggestion / something to think
Sure. One issue of course is that there may be different behavior depending on
if it fails the first time or a subsequent time, and we ideally want to test
both.
I thought about ways to make this more controllable, like maybe you could
specify a number of times the rpc should succeed before it starts to fail, but
its hard to come up with something that covers all possible cases and isn't
super complicated.
For now I think its best to keep it fairly simple and rely on a combination of
randomness and running a bunch of times to ensure good coverage.
And, all of the tests here have been designed where we really expect that
they'll only run this loop once most of the time.
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@67
PS3, Line 67: # times. Each test in this file has at least a 50% chance of
hitting the action per
> should there be a limit or a timeout to the number of times a query is atte
Done
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@76
PS3, Line 76: except ImpalaBeeswaxException as e:
> is this necessary? if the while loop exits this should always be true, righ
That was true in this version, but with adding a limit to the number of
iterations above, I'll leave this here.
--
To view, visit http://gerrit.cloudera.org:8080/14641
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id:
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/impala-service-pool.cc@207 PS3, Line 207: FailAndReleaseRpc perhaps not in this patch, but would it be good to have an option to just completely ignore the incoming RPC call? I think right now this responds with an error, but it would be good to test what happens when there is no RPC response as well. http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@63 PS3, Line 63: execute_test_query > it seems like this while loop in this method is necessary because there is Not sure how easy this would be, more of a suggestion / something to think about. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 13 Nov 2019 02:11:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Sahil Takiar has posted comments on this change. (
http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options
..
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14641/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14641/3//COMMIT_MSG@26
PS3, Line 26: RPC_SERVICE_POOL
IMPALA_SERVICE_POOL?
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/rpc/rpc-mgr.cc@193
PS3, Line 193: DCHECK(IsResolvedAddress(address_));
move this to Init? since that is when address is passed in?
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/14641/3/be/src/util/debug-util.cc@400
PS3, Line 400: if (ImpaladMetrics::DEBUG_ACTION_NUM_FAIL != nullptr) {
this should never be nullptr right? add a DCHECK asserting it is never a
nullptr?
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@63
PS3, Line 63: must_fail=True
is this used anywhere?
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@63
PS3, Line 63: execute_test_query
it seems like this while loop in this method is necessary because there is only
a certain probability that a failure is actually injected. the reason the
probability is necessary is that we don't want all RPC attempts to fail. would
a better way to do this be something similar to
https://github.com/apache/impala/commit/19cb8dc1c1c2247e91adc4bf62cab27a7c1e4381#diff-ab4af79ee4df02bf95d708a1d207f79aR189-R201
maybe there is a generic way to create a FAIL_FIRST debug action?
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@67
PS3, Line 67: while self._get_num_fails(impalad) == 0:
should there be a limit or a timeout to the number of times a query is
attempted? otherwise if this test breaks this loop may never exit
http://gerrit.cloudera.org:8080/#/c/14641/3/tests/custom_cluster/test_rpc_exception.py@76
PS3, Line 76: assert self._get_num_fails(impalad) > 0
is this necessary? if the while loop exits this should always be true, right?
--
To view, visit http://gerrit.cloudera.org:8080/14641
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029
Gerrit-Change-Number: 14641
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Wed, 13 Nov 2019 01:56:42 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 3: (3 comments) A few more comments. Planning to take another look later today. http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@395 PS2, Line 395: } : string error_msg = tokens.size() == 3 ? : tokens[2] : > I'm not sure what you mean. Its a property of this particular debug action Yeah, didn't see the check for "iequals(cmd, "FAIL")" above. Although, I think the pattern in Impala is to document the return type for each method, so would be nice to document that for this method as well. http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@92 PS2, Line 92: ::...::@@@..." > Sure, since the debug action in this patch is being passed in as a command Yeah, maybe passing in a path to a file that contains JSON is the right way to do it. Just a thought really. I think another way to make it easier to understand how to use DEBUG_ACTIONS, would be to include some examples. Can you add a few? At least, a few relevant to the changes you are making. http://gerrit.cloudera.org:8080/#/c/14641/3/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/3/common/thrift/ImpalaService.thrift@101 PS3, Line 101: and error should mention that the DebugAction might change code paths depending on the value of error - e.g. in impala-service-pool.cc QueueInboundCall, it either calls FailAndReleaseRpc or RejectTooBusy depending on the value of error. Right now, it sounds like it just returns a Status(error), and thats it. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 12 Nov 2019 17:31:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4993/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Sat, 09 Nov 2019 00:41:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Tauber-Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options
..
Patch Set 3:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/14641/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14641/2//COMMIT_MSG@25
PS2, Line 25: optional 'error
> do you mean IMPALA_SERVICE_POOL?
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.h@115
PS2, Line 115: std::string hostname_;
: std::string p
> nit: use std::string
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.cc@194
PS2, Line 194: mulating rpc errors. To use, specify:
: // --debug_actions=IMPALA_SERVICE_POOL:::
> would be good to document this some more, it is basically a DebugAction tha
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/rpc-mgr-test.cc@322
PS2, Line 322: Ping
> what is Ping suppose to mean?
Its the name of a particular rpc that is part of the Ping service, which is a
toy service we use for testing, see above in this test.
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@395
PS2, Line 395: }
: string error_msg = tokens.size() == 3 ?
: tokens[2] :
> should returning tokens[2] only be done if the action is FAIL? might be goo
I'm not sure what you mean. Its a property of this particular debug action that
it takes the parameter 'error message' and returns it in the error, which is
documented here in the comment above and in ImpalaService.thrift. Other debug
actions return on OK status.
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@399
PS2, Line 399:
: if (ImpaladMetrics::DEBUG_ACTION_NUM_FAIL != nullptr) {
: ImpaladMetrics::DEBUG_ACTION_NUM_FAIL->Increment(1l);
: }
: return Status(TErrorCode::INTERNAL_ERROR, error_msg);
: } else {
: D
> is this thread safe?
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@92
PS2, Line 92: ::...::@@@..."
> as these become more and more complex, it might be worth re-visiting the fo
Sure, since the debug action in this patch is being passed in as a command line
flag its a little awkward to make it JSON or Thrift but its worth thinking
about.
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@100
PS2, Line 100: [@] returns
> this is optional right? should we document the behavior if it is omitted?
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@27
PS2, Line 27: # DataStreamService rpc names
: TRANSMIT_DATA_RPC = "TransmitData"
: END_DATA_STREAM_RPC = "EndDataStream"
:
: # Error to specify for ImpalaServicePool to reject rpcs with a
'server too busy' error.
: REJECT_TOO_BUSY_MSG = "REJECT_TOO_BUSY"
:
> can you document these all a bit?
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@76
PS2, Line 76: assert self._get_num_fails(impalad) > 0
:
: def _get_fail_action(rpc, error=None, port=KRPC_PORT, p=0.1):
> can you document this a bit more and explain what exactly this debug action
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88
PS2, Line 88: @CustomCluster
> define this and END_ERROR at the top of the file, next to REJECT_TOO_BUSY_M
Done
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88
PS2, Line 88: Suite.with_args("--
> is this and END_DATA_STREAM_ERROR suppose to be defined in this patch?
Done
--
To view, visit http://gerrit.cloudera.org:8080/14641
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029
Gerrit-Change-Number: 14641
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14641 to look at the new patch set (#3). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action IMPALA_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action RPC_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 26 files changed, 225 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/3 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Sahil Takiar has posted comments on this change. (
http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options
..
Patch Set 2:
(12 comments)
First pass. Mostly clarification questions + requests for documentation.
http://gerrit.cloudera.org:8080/#/c/14641/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14641/2//COMMIT_MSG@25
PS2, Line 25: RPC_SERVICE_POOL
do you mean IMPALA_SERVICE_POOL?
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.h@115
PS2, Line 115: string hostname_;
: string port_;
nit: use std::string
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.cc@194
PS2, Line 194: DebugAction(FLAGS_debug_actions, "IMPALA_SERVICE_POOL",
: {hostname_, port_, c->remote_method().method_name()})
would be good to document this some more, it is basically a DebugAction that
matches on IMPALA_SERVICE_POOL, the hostname, port, and the remote method name?
what is the remote method name suppose to represent?
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/rpc-mgr-test.cc@322
PS2, Line 322: Ping
what is Ping suppose to mean?
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@395
PS2, Line 395: string error_msg = tokens.size() == 3 ?
: tokens[2] :
: Substitute("Debug Action: $0:$1", components[0],
action_str);
should returning tokens[2] only be done if the action is FAIL? might be good to
document what error message is returned, either here or in the method
declaration.
http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@399
PS2, Line 399: IntCounter* metric =
:
ExecEnv::GetInstance()->metrics()->FindMetricForTesting(
: "impala.debug_action.fail");
: if (!metric) {
: metric =
:
ExecEnv::GetInstance()->metrics()->AddCounter("impala.debug_action.fail", 0);
: }
is this thread safe?
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@92
PS2, Line 92: ::...::@@@..."
as these become more and more complex, it might be worth re-visiting the format
we define debug actions in. probably out of scope for this JIRA, but might be
worth filing a separate JIRA to revisit how debug actions are defined. maybe
defining them as JSON or Thrift objects would be cleaner.
http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@100
PS2, Line 100: [@]
this is optional right? should we document the behavior if it is omitted?
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@27
PS2, Line 27: # DataStreamService
: TRANSMIT_DATA_RPC = "TransmitData"
: END_DATA_STREAM_RPC = "EndDataStream"
:
: REJECT_TOO_BUSY_MSG = "REJECT_TOO_BUSY"
:
: KRPC_PORT = 27002
can you document these all a bit?
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@76
PS2, Line 76: def _get_fail_action(rpc, error, p=0.1, port=KRPC_PORT):
: return
"IMPALA_SERVICE_POOL:127.0.0.1:{port}:{rpc}:FAIL@{probability}@{error}" \
: .format(rpc=rpc, error=error, probability=p, port=port)
can you document this a bit more and explain what exactly this debug action is
suppose to do.
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88
PS2, Line 88: TRANSMIT_ERROR
define this and END_ERROR at the top of the file, next to REJECT_TOO_BUSY_MSG?
would be good to document them all a bit as well
http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88
PS2, Line 88: TRANSMIT_DATA_ERROR
is this and END_DATA_STREAM_ERROR suppose to be defined in this patch?
--
To view, visit http://gerrit.cloudera.org:8080/14641
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4954/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 05 Nov 2019 22:38:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4952/ : 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/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 05 Nov 2019 22:24:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py File tests/custom_cluster/test_blacklist.py: http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py@118 PS1, Line 118: > flake8: E501 line too long (119 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py@131 PS1, Line 131: > flake8: E501 line too long (105 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py@134 PS1, Line 134: > flake8: F821 undefined name 'ImpalaBeeswaxException' Done http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_rpc_exception.py@89 PS1, Line 89: > flake8: E301 expected 1 blank line, found 0 Done http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_rpc_exception.py@107 PS1, Line 107: E > flake8: E301 expected 1 blank line, found 0 Done -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 05 Nov 2019 21:44:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14641 to look at the new patch set (#2). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action RPC_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action RPC_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_rpc_exception.py 24 files changed, 204 insertions(+), 279 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/2 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14641 Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, Impala had a very simple 'fault injection' framework for simulating failed rpcs between impalads. With the move to KRPC, that framework was not carried over, and we lost the ability to test certain failure scenarios. This patch reintroduces this functionality. It removes the prior fault injection framework in favor of the existing debug action framework, which is more flexible. To facilitate this, a few modifications are made to the debug action framework: - In addition to matching on a label, debug actions may now match on optional arguments. In this patch, the debug action RPC_SERVICE_POOL takes the arguments 'host', 'port', and 'rpc name' to allow simulating the failure of specific rpcs to specific impalads. - The FAIL action now takes an optional 'error message' parameter. In this patch, the debug action RPC_SERVICE_POOL uses this to simulate different types of rpc errors, eg. 'service too busy'. - The FAIL action increments a metric, 'impala.debug_action.fail', so that tests can check that it has actually been hit. Prior to this patch the tests in test_rpc_exception.py where all passing spuriously as the faults they were supposed to be testing were no longer being injected. This patch uses these new mechanisms to introduce tests that simulate failures in DataStreamService rpcs. Follow up patches will add test cases for ControlService rpcs. Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 --- M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/backend-client.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/service/impala-internal-service.cc M be/src/testutil/CMakeLists.txt D be/src/testutil/fault-injection-util.cc D be/src/testutil/fault-injection-util.h M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M common/thrift/metrics.json M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_blacklist.py M tests/custom_cluster/test_rpc_exception.py 25 files changed, 225 insertions(+), 281 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14641/1 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py File tests/custom_cluster/test_blacklist.py: http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py@118 PS1, Line 118: x flake8: E501 line too long (119 > 90 characters) http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py@131 PS1, Line 131: r flake8: E501 line too long (105 > 90 characters) http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_blacklist.py@134 PS1, Line 134: I flake8: F821 undefined name 'ImpalaBeeswaxException' http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_rpc_exception.py@89 PS1, Line 89: @ flake8: E301 expected 1 blank line, found 0 http://gerrit.cloudera.org:8080/#/c/14641/1/tests/custom_cluster/test_rpc_exception.py@107 PS1, Line 107: @ flake8: E301 expected 1 blank line, found 0 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Tue, 05 Nov 2019 21:40:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Marshall has abandoned this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Abandoned we've decided to go in a different direction -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/12297 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options
..
Patch Set 1:
(6 comments)
Thanks for working on this patch. My general comment is to consider moving away
from the existing "fake" fault injection framework in Thrift and use debug
actions to simulate scenarios in which we may actually fail to exercise the
entire RPC stack better.
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/rpc/impala-control-service-proxy.h
File be/src/rpc/impala-control-service-proxy.h:
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/rpc/impala-control-service-proxy.h@44
PS5, Line 44:
As mentioned elsewhere, this kind of artificial fault injection doesn't seem to
be too useful.
http://gerrit.cloudera.org:8080/#/c/12297/3/be/src/rpc/impala-control-service-proxy.h
File be/src/rpc/impala-control-service-proxy.h:
http://gerrit.cloudera.org:8080/#/c/12297/3/be/src/rpc/impala-control-service-proxy.h@43
PS3, Line 43:
Not needed. Same below.
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/rpc/rpc-mgr.inline.h
File be/src/rpc/rpc-mgr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/rpc/rpc-mgr.inline.h@65
PS5, Line 65:
:
:
:
:
:
:
:
:
:
:
:
:
It appears that this send vs recv debug actions were carried over from Thrift
implementation.
Retrospectively, the "fault injection" we did with Thrift was quite hacky (I am
the culprit here) and it stemmed from the total lack of fault injection
testing back then for exercising the error paths in Thrift RPC.
As part of the KRPC development, we invested in proper fault injection testing
by truly pausing the Impala and artificially creates various failure scenario.
This allows a more extensive exercise across the entire RPC stack instead of
just exercising the RPC handlers at the client and server sides.
With the fault injection framework, it seems to be not too useful to continue
with this path of artificial fault injection via debug action we used to do
with Thrift.
Instead, we may want to rethink the fault injection testing with KRPC. In
particular, it may exercise the code better by doing some of the followings:
- use debug actions to inject random delays in the RPC handlers. This is
particularly useful for RPCs with timeout
- use debug actions to randomly reject some of the incoming RPCs in
ImpalaServicePool
- use debug actions to respond with error status in the RPC handlers. The
errors will be specific to each RPC handler (e.g. deserialization error of
Thrift profiles, deserialization errors of RowBatch)
- debug action to force some incoming RPCs to use deferred queue in
KrpcDataStreamRecvr
- (experimental / dangerous) "randomly" corrupt the incoming RPC payloads in
ImpalaServicePool.
- inject delay in RPC callback in the client side to simulate an overloaded
client
The above are some examples I can think of right now.
For other failures, we may need to rely on the fault injection framework:
- use iptables to drop all incoming packets to the RPC port from a particular
host. This simulates a host which was powered off or network partitions
- Restart remote Impalad will trigger the behavior of broken connections (by
sending a RST packet)
- Send SIGSTOP to remote Impalad (which we already do in the fault injection
framework) to simulate non-responsive Impalad
- other ideas...
In general, my suggestion is to use debug actions to simulate failure which can
actually happen instead of using this artificial fault injection which seems a
bit meaningless at this point.
http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/query-state.h@314
PS1, Line 314: proxy_
> I assume that if I change the class name back to something ending in "Proxy
Yup.
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/service/control-service.cc@171
PS5, Line 171: if (qs.get() == nullptr) {
: Status status(ErrorMsg(TErrorCode::INTERNAL_ERROR,
Should this be converted to debug action ?
http://gerrit.cloudera.org:8080/#/c/12297/5/be/src/service/control-service.cc@186
PS5, Line 186:
:
Should this be converted to debug action ?
--
To view, visit http://gerrit.cloudera.org:8080/12297
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941
Gerrit-Change-Number: 12297
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Andrew
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2758/ : 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/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 12 Apr 2019 20:14:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2743/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 11 Apr 2019 23:39:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Andrew Sherman, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12297 to look at the new patch set (#4). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, we had fault injection options for backend rpcs implemented in ImpalaBackendClient. With the move the krpc, we lost some of those options. This patch reintroduces equivalent options, except that they now utilize the existing DebugAction framework, which is more flexible and powerful than the FAULT_INJECTION framework used previously. Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 --- A be/src/rpc/impala-control-service-proxy.h A be/src/rpc/impala-data-stream-service-proxy.h M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/rpc-mgr.inline.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/krpc-data-stream-sender.cc M be/src/runtime/query-state.cc M be/src/runtime/query-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/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/util/CMakeLists.txt A be/src/util/krpc-debug-util.cc A be/src/util/krpc-debug-util.h M tests/custom_cluster/test_restart_services.py M tests/query_test/test_cancellation.py 20 files changed, 487 insertions(+), 133 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12297/4 -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12297/4/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/12297/4/tests/custom_cluster/test_restart_services.py@337 PS4, Line 337: flake8: E203 whitespace before ':' -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 11 Apr 2019 23:13:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2582/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 29 Mar 2019 00:14:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Andrew Sherman, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12297 to look at the new patch set (#3). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, we had fault injection options for backend rpcs implemented in ImpalaBackendClient. With the move the krpc, we lost some of those options. This patch reintroduces equivalent options, except that they now utilize the existing DebugAction framework, which is more flexible and powerful than the FAULT_INJECTION framework used previously. Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 --- M be/src/common/global-flags.cc A be/src/rpc/impala-control-service-proxy.h A be/src/rpc/impala-data-stream-service-proxy.h M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/rpc-mgr.inline.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/krpc-data-stream-sender.cc M be/src/runtime/query-state.cc M be/src/runtime/query-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/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/util/CMakeLists.txt M be/src/util/debug-util.cc M be/src/util/debug-util.h A be/src/util/krpc-debug-util.cc A be/src/util/krpc-debug-util.h M tests/custom_cluster/test_restart_services.py M tests/query_test/test_cancellation.py 23 files changed, 475 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12297/3 -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 3: Rebased on top of the latest version of https://gerrit.cloudera.org/#/c/12672/, still not really ready for full review until that patch goes in -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 28 Mar 2019 23:34:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: (1 comment) I will try to get https://gerrit.cloudera.org/#/c/12672/ into submittable shape, http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h File be/src/runtime/krpc-backend-client.h: http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h@28 PS1, Line 28: class KrpcBackendClient : public ControlServiceProxy > > Should this class show that it is a decorated form of Thanks this all seems fine. -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 07 Mar 2019 23:51:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2388/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 07 Mar 2019 20:11:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 2: (3 comments) This is not really ready for full review until https://gerrit.cloudera.org/#/c/12672/ is submitted and it is rebased on top, I just updated it so that reviewers of that patch can see what I've done for the async case. http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h File be/src/runtime/krpc-backend-client.h: http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h@27 PS1, Line 27: > Please add a class level comment. Done http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h@28 PS1, Line 28: > Should this class show that it is a decorated form of > ControlServiceProxy, perhaps by having ControlServiceProxy as part > of it's name? ControlServiceProxyInjected? ControlServiceProxyDecorated? Good idea. "Injected" and "Decorated" seem awkward to me, is it clear enough if I go with ImpalaControlServiceProxy? > > I think this change introduces a convention, that we expect > ControlServiceProxy methods to be overridden in KrpcBackendClient? > If that's so then a comment somewhere should say that. Is it in > fact now a mistake to use ControlServiceProxy? Addressed in the class comment. > > It seems like KrpcBackendClient is mostly boilerplate. Did you > consider extending protoc-gen-krpc.cc to either generate the > decorated methods in ControlServiceProxy, or to generate > KrpcBackendClient? As discussed, while this is an interesting idea, the Kudu people don't have much interest in directly adding such a feature to krpc, and for now at least the benefit of automating this boiler-plate is probably not worth the work. http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/query-state.h@314 PS1, Line 314: eProxy > Will client_ be a more appropriate name ? Same for other places. I assume that if I change the class name back to something ending in "Proxy" that you're fine leaving this as is? -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 07 Mar 2019 19:33:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Hello Michael Ho, Andrew Sherman, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12297 to look at the new patch set (#2). Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, we had fault injection options for backend rpcs implemented in ImpalaBackendClient. With the move the krpc, we lost some of those options. This patch reintroduces equivalent options, except that they now utilize the existing DebugAction framework, which is more flexible and powerful than the FAULT_INJECTION framework used previously. Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 --- A be/src/rpc/impala-control-service-proxy.h A be/src/rpc/impala-data-stream-service-proxy.h M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/krpc-data-stream-sender.cc M be/src/runtime/query-state.cc M be/src/runtime/query-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/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/util/CMakeLists.txt A be/src/util/krpc-debug-util.cc A be/src/util/krpc-debug-util.h M tests/custom_cluster/test_restart_services.py M tests/query_test/test_cancellation.py 19 files changed, 424 insertions(+), 108 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12297/2 -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: Any update ? -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Mon, 25 Feb 2019 23:27:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: (1 comment) This all looks good but I have questions... http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h File be/src/runtime/krpc-backend-client.h: http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h@28 PS1, Line 28: class KrpcBackendClient : public ControlServiceProxy > Hmm...what about DataStreamService ? This seems to be a client for ControlS Should this class show that it is a decorated form of ControlServiceProxy, perhaps by having ControlServiceProxy as part of it's name? ControlServiceProxyInjected? ControlServiceProxyDecorated? I think this change introduces a convention, that we expect ControlServiceProxy methods to be overridden in KrpcBackendClient? If that's so then a comment somewhere should say that. Is it in fact now a mistake to use ControlServiceProxy? It seems like KrpcBackendClient is mostly boilerplate. Did you consider extending protoc-gen-krpc.cc to either generate the decorated methods in ControlServiceProxy, or to generate KrpcBackendClient? -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 30 Jan 2019 20:24:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1922/ : 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/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 30 Jan 2019 01:23:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12297 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h File be/src/runtime/krpc-backend-client.h: http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h@27 PS1, Line 27: Please add a class level comment. http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/krpc-backend-client.h@28 PS1, Line 28: class KrpcBackendClient : public ControlServiceProxy Hmm...what about DataStreamService ? This seems to be a client for ControlService's RPCs only ? http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/12297/1/be/src/runtime/query-state.h@314 PS1, Line 314: proxy_ Will client_ be a more appropriate name ? Same for other places. -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 30 Jan 2019 00:55:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12297 Change subject: IMPALA-8138: Reintroduce rpc debugging options .. IMPALA-8138: Reintroduce rpc debugging options In the past, we had fault injection options for backend rpcs implemented in ImpalaBackendClient. With the move the krpc, we lost some of those options. This patch reintroduces equivalent options, except that they now utilize the existing DebugAction framework, which is more flexible and powerful than the FAULT_INJECTION framework used previously. Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h A be/src/runtime/krpc-backend-client.h M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/service/control-service.cc M be/src/service/control-service.h M tests/query_test/test_cancellation.py 8 files changed, 72 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12297/1 -- To view, visit http://gerrit.cloudera.org:8080/12297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2046cb9dadf846ea90c04e95781b2bbde3325941 Gerrit-Change-Number: 12297 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
