[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has submitted this change and it was merged. Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specified _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. This adds a tests that has multiple threads trying to execute the same rpc, and makes sure each one was executed exactly once. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Reviewed-on: http://gerrit.cloudera.org:8080/3192 Tested-by: Kudu Jenkins Reviewed-by: Jean-Daniel Cryans --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 21 files changed, 372 insertions(+), 53 deletions(-) Approvals: Jean-Daniel Cryans: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 27 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Jean-Daniel Cryans has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 26: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 26 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 26: Build Started http://104.196.14.100/job/kudu-gerrit/2376/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 26 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has posted comments on this change.
Change subject: Integrate the ResultTracker into the rpc subsystem
..
Patch Set 24:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/3192/24/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:
Line 456: "result_tracker_ = result_tracker;"
> this can use an initializer list, right?
well, this is initializing a protected member of a superclass, to be able to
use an initializer list I'd need to add a new ctor the and then call that ctor
from here. Mind if we just leave it?
http://gerrit.cloudera.org:8080/#/c/3192/24/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:
PS24, Line 59: We don't use CalculatorServiceRpc
> not sure what class this is referring to. I guess this is coming in a later
I can't remember either. All of them use that service. removed.
Line 68: atomic_int* attempt_nos) {
> not following why this is atomic. you're fetch_add()ing to it from the main
Done
Line 70: client_sleep_for_ms = client_sleep;
> above two lines can be from an initializer list
Done
PS24, Line 83: a the
> typo
Done
Line 91: uint64_t client_sleep_for_ms;
> make these above two const
Done
Line 100: string client_id_;
> this is just a constant, right? can you just make it a const char* kClientI
Done
--
To view, visit http://gerrit.cloudera.org:8080/3192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#26). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specified _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. This adds a tests that has multiple threads trying to execute the same rpc, and makes sure each one was executed exactly once. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 21 files changed, 372 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/26 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 26 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Todd Lipcon has posted comments on this change.
Change subject: Integrate the ResultTracker into the rpc subsystem
..
Patch Set 24:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/3192/24/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:
Line 456: "result_tracker_ = result_tracker;"
this can use an initializer list, right?
http://gerrit.cloudera.org:8080/#/c/3192/24/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:
PS24, Line 59: We don't use CalculatorServiceRpc
not sure what class this is referring to. I guess this is coming in a later
patch?
Line 68: atomic_int* attempt_nos) {
not following why this is atomic. you're fetch_add()ing to it from the main
thread in the constructor, and then it's not accessed from the started threads.
Why not just take an int attempt_num here?
Line 70: client_sleep_for_ms = client_sleep;
above two lines can be from an initializer list
PS24, Line 83: a the
typo
Line 91: uint64_t client_sleep_for_ms;
make these above two const
Line 100: string client_id_;
this is just a constant, right? can you just make it a const char* kClientId?
--
To view, visit http://gerrit.cloudera.org:8080/3192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#25). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specified _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. This adds a tests that has multiple threads trying to execute the same rpc, and makes sure each one was executed exactly once. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 21 files changed, 376 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/25 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 25 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 25: Build Started http://104.196.14.100/job/kudu-gerrit/2374/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 25 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#24). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specified _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. This adds a tests that has multiple threads trying to execute the same rpc, and makes sure each one was executed exactly once. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 21 files changed, 390 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/24 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 24 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 24: Build Started http://104.196.14.100/job/kudu-gerrit/2362/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 24 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 23: Build Started http://104.196.14.100/job/kudu-gerrit/2336/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 23 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#23). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specified _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. This adds a tests that has multiple threads trying to execute the same rpc, and makes sure each one was executed exactly once. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 21 files changed, 388 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/23 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 23 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 19: (7 comments) http://gerrit.cloudera.org:8080/#/c/3192/19//COMMIT_MSG Commit Message: PS19, Line 11: specificed > typo Done http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/CMakeLists.txt File src/kudu/rpc/CMakeLists.txt: Line 111: kudu_common > is this still needed? this back-dependency strikes me as evil dom't think so, good catch. removed http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 189: (*map)["result_tracker"] = "result_tracker"; > how about: (*map)["result_tracker"] = track_result ? "result_tracker" : "nu Done http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: Line 23: #include "kudu/rpc/result_tracker.h" > forward decl Done Line 181: // If this returns true, both GetResultTracker() request_id() should return non > - missing word "and" Done PS19, Line 186: Get > style: no "Get" Done http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/service_if.h File src/kudu/rpc/service_if.h: Line 58: scoped_refptr result_tracker; > now that the result tracker is service-wide, does it really make sense to h made the result tracker a member of the GeneratedServiceIf class and made the RpcMethodInfo field a bool. -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 22: Build Started http://104.196.14.100/job/kudu-gerrit/2275/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 22 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 21: Build Started http://104.196.14.100/job/kudu-gerrit/2255/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 21 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 20: Build Started http://104.196.14.100/job/kudu-gerrit/2251/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 20 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#20). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. This adds a tests that has multiple threads trying to execute the same rpc, and makes sure each one was executed exactly once. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 21 files changed, 390 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/20 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 20 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Todd Lipcon has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 19: (7 comments) I think this commit would make more sense if you also pulled in the changes to RetriableRpc from the 'integrate with client', and then merged in the new test cases as well, so it's a standalone change to the RPC system along with its tests http://gerrit.cloudera.org:8080/#/c/3192/19//COMMIT_MSG Commit Message: PS19, Line 11: specificed typo http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/CMakeLists.txt File src/kudu/rpc/CMakeLists.txt: Line 111: kudu_common is this still needed? this back-dependency strikes me as evil http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 189: (*map)["result_tracker"] = "result_tracker"; how about: (*map)["result_tracker"] = track_result ? "result_tracker" : "nullptr" ? http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: Line 23: #include "kudu/rpc/result_tracker.h" forward decl Line 181: // If this returns true, both GetResultTracker() request_id() should return non - missing word "and" - hyphenate "non-null" PS19, Line 186: Get style: no "Get" http://gerrit.cloudera.org:8080/#/c/3192/19/src/kudu/rpc/service_if.h File src/kudu/rpc/service_if.h: Line 58: scoped_refptr result_tracker; now that the result tracker is service-wide, does it really make sense to have here vs just having a bool and grabbing the member of the service class? if not, another thought: RpcContext::ResultTracker() can call through to this class to grab the method info (and avoid an extra ref/deref on the ResultTracker ref count) -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 467: "mi->result_tracker.reset($result_tracker$);\n" > pondered this a bit more, seems like, for now it won't make a difference ei actually done this -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 19: Build Started http://104.196.14.100/job/kudu-gerrit/2186/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#19). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. A follow up patch tests this in integration with the client. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/master/master.cc M src/kudu/master/master_service.cc M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h M src/kudu/server/generic_service.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/remote_bootstrap_service.cc M src/kudu/tserver/remote_bootstrap_service.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 19 files changed, 185 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/19 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 18: Build Started http://104.196.14.100/job/kudu-gerrit/2160/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 17: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 17: unrelated flake in RemoteKsckTest.TestChecksumTimeout -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 17: Build Started http://104.196.14.100/job/kudu-gerrit/2132/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#16). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. A follow up patch tests this in integration with the client. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 8 files changed, 138 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/16 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 16: Build Started http://104.196.14.100/job/kudu-gerrit/2049/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 15: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2041/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 15: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2030/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
David Ribeiro Alves has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 467: "mi->result_tracker.reset($result_tracker$);\n" > we do, why doesn't it seem right? easier to track where memory goes , maps pondered this a bit more, seems like, for now it won't make a difference either way and would like to have garbage collection working before I make the change to a single result tracker, if that's ok. -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 15: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2024/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 15: Build Started http://104.196.14.100/job/kudu-gerrit/2021/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem .. Patch Set 14: Build Started http://104.196.14.100/job/kudu-gerrit/2011/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#14). Change subject: Integrate the ResultTracker into the rpc subsystem .. Integrate the ResultTracker into the rpc subsystem This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. A follow up patch tests this in integration with the client. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 8 files changed, 139 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/14 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 13: Build Started http://104.196.14.100/job/kudu-gerrit/2004/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
David Ribeiro Alves has posted comments on this change.
Change subject: Integrate the ResultTracker into the rpc subsystem and add a
test
..
Patch Set 12: Verified+1
(12 comments)
http://gerrit.cloudera.org:8080/#/c/3192/6//COMMIT_MSG
Commit Message:
Line 10: to specify a new method option when defining rpc service methods.
> I think you missed this.
hrm, you're right. I could swear I had fixed this. must have lost a patch along
the way. Done
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/common/common.proto
File src/kudu/common/common.proto:
Line 316
> I dont think this should be in 'common'. 'common' is for data-model type st
Done
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/CMakeLists.txt
File src/kudu/rpc/CMakeLists.txt:
Line 89: rpc_header_proto
> oh, now I see why it builds. This dependency seems misplaced. (eg keep in m
Done
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:
Line 46: #include "kudu/rpc/rpc_header.pb.h"
> nit: sorting
Done
Line 467: "mi->result_tracker.reset($result_tracker$);\n"
> we have a separate ResultTracker per RPC type? that doesn't seem right.
we do, why doesn't it seem right? easier to track where memory goes , maps will
be smaller so faster. locks will be less contended... why wouldn't we want to
have one per rpc?
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:
PS11, Line 73: random amount
> update
Done
Line 134: vector> adders;
> use unique_ptr?
Done
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
Line 288: std::atomic_int exactly_once_test_val_;
> hrm, is this atomic? haven't seen this
yeah it's typedefd, want me to change it?
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc_context.cc
File src/kudu/rpc/rpc_context.cc:
PS11, Line 66: call_->RespondSuccess(*response_pb_);
: delete this;
:
> per the comment on the other review, it seems weird we now have this code s
well yeah, for the case when the result aren't being tracked. ideally we'd like
to at least coalesce the log statements at least, was kinda bugging me but
didn't spend too much time on it. Any ideas?
http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rtest.proto
File src/kudu/rpc/rtest.proto:
Line 129: }
> add something to design-docs/rpc.md about this feature?
Done
http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:
PS6, Line 96: req.release(),
:resp,
:
call->header().has_request_id() ? method_info->result_tracker :
:
nullptr);
:
> Missed this?
Done
Line 116: } else {
> Log the state too.
Done
--
To view, visit http://gerrit.cloudera.org:8080/3192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#12). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new method option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M docs/design-docs/rpc.md M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 12 files changed, 356 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/12 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 12: Build Started http://104.196.14.100/job/kudu-gerrit/1961/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Todd Lipcon has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/common/common.proto File src/kudu/common/common.proto: Line 316: optional bool track_rpc_result = 50006 [default=false]; I dont think this should be in 'common'. 'common' is for data-model type stuff. Perhaps 'rpc_header.proto' or a new proto file in the rpc/ subdir. (rpc doesn't depend on common, so actually slightly surprised this builds properly) http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/CMakeLists.txt File src/kudu/rpc/CMakeLists.txt: Line 89: kudu_common_proto oh, now I see why it builds. This dependency seems misplaced. (eg keep in mind that Impala's thinking about using krpc, so we shouldn't have upwards dependencies from RPC into common/) http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 46: #include "kudu/common/common.pb.h" nit: sorting Line 467: "mi->result_tracker.reset($result_tracker$);\n" we have a separate ResultTracker per RPC type? that doesn't seem right. http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc-stress-test.cc File src/kudu/rpc/rpc-stress-test.cc: PS11, Line 73: random amount update Line 134: vector adders; use unique_ptr? http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 288: std::atomic_int exactly_once_test_val_; hrm, is this atomic? haven't seen this http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc_context.cc File src/kudu/rpc/rpc_context.cc: PS11, Line 66: call_->RespondSuccess(*response_pb_); : delete this; : per the comment on the other review, it seems weird we now have this code split between ResultTracker and here. http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rtest.proto File src/kudu/rpc/rtest.proto: Line 129: option (kudu.track_rpc_result) = true; add something to design-docs/rpc.md about this feature? -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Adar Dembo has posted comments on this change.
Change subject: Integrate the ResultTracker into the rpc subsystem and add a
test
..
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3192/6//COMMIT_MSG
Commit Message:
Line 10: to specify a new mehtod option when defining rpc service methods.
> Nit: method
I think you missed this.
http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:
PS6, Line 96: if (!call->header().has_request_id()) {
: ctx = new RpcContext(call, req.release(), resp, nullptr);
: } else {
: ctx = new RpcContext(call, req.release(), resp,
method_info->result_tracker);
: }
> Convert into a ternary:
Missed this?
Line 116: }
> Log the state too.
And this.
--
To view, visit http://gerrit.cloudera.org:8080/3192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 11: Build Started http://104.196.14.100/job/kudu-gerrit/1909/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 10: Build Started http://104.196.14.100/job/kudu-gerrit/1897/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#10). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 330 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/10 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#9). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 325 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/9 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 9: Build Started http://104.196.14.100/job/kudu-gerrit/1890/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 8: Build Started http://104.196.14.100/job/kudu-gerrit/1885/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#7). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 325 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/7 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Kudu Jenkins has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 7: Build Started http://104.196.14.100/job/kudu-gerrit/1882/ -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
David Ribeiro Alves has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Patch Set 6: (1 comment) This is still missing a parallel test for the failure case. http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: Line 103: ResultTracker::RpcState state = ctx->GetResultTracker()->CheckDuplicate( > It's interesting that 'req' isn't considered at all in CheckDuplicate(). We that's on purpose. storing the requests is bound to consume much more memory that storing the responses so we need to "trust" that the client sends the same request with the same seq no. In any case the worst that can happen is that the client gets the "wrong" response, if he sent a requests with a previously used seq no. -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Adar Dembo has posted comments on this change.
Change subject: Integrate the ResultTracker into the rpc subsystem and add a
test
..
Patch Set 6:
(9 comments)
It's becoming increasingly clear to me that Todd needs to take a deep look at
this series; there's too much here that I don't know.
http://gerrit.cloudera.org:8080/#/c/3192/6//COMMIT_MSG
Commit Message:
Line 10: to specify a new mehtod option when defining rpc service methods.
Nit: method
http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/common/common.proto
File src/kudu/common/common.proto:
Line 313: // An option for RPC methods that allows to set whether that method's
Why define this here and not in rpc_header.proto? Wouldn't doing that mean
proto-gen-krpc won't depend on kudu_common?
http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:
Line 46: #include "kudu/common/common.pb.h"
Nit: should come before gutil.
Line 172:
Nit: extra line?
http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/rpc_context.h
File src/kudu/rpc/rpc_context.h:
Line 178: bool AreResultsTracked() const { return result_tracker_.get() !=
nullptr; }
I don't think this is necessary given the below method, which is very easy to
use to test for the presence of a tracker.
Line 180: const scoped_refptr GetResultTracker() const {
Shouldn't this return a cref and return the result_tracker_ as-is, instead of
calling get()?
Also, call it result_tracker()?
http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:
Line 96: if (!call->header().has_request_id()) {
: ctx = new RpcContext(call, req.get(), resp, nullptr);
: } else {
: ctx = new RpcContext(call, req.get(), resp,
method_info->result_tracker);
: }
Convert into a ternary:
RpcContext* ctx = new RpcContext(call, req.get(), resp,
call->header().has_request_id() ? method_info->result_tracker : nullptr);
Line 103: ResultTracker::RpcState state =
ctx->GetResultTracker()->CheckDuplicate(
It's interesting that 'req' isn't considered at all in CheckDuplicate(). We're
relying entirely on seq_no() instead, I believe.
Line 116: LOG(FATAL) << "Unknown state.";
Log the state too.
--
To view, visit http://gerrit.cloudera.org:8080/3192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#6). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 324 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/6 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#5). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 326 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/5 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#4). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 324 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/4 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#3). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 324 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/3 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3192 to look at the new patch set (#2). Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 321 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/2 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test
David Ribeiro Alves has uploaded a new change for review. http://gerrit.cloudera.org:8080/3192 Change subject: Integrate the ResultTracker into the rpc subsystem and add a test .. Integrate the ResultTracker into the rpc subsystem and add a test This integrates the ResultTracker into the rpc subsystem by allowing to specify a new mehtod option when defining rpc service methods. When this method option is specificed _and_ the call's rpc header has a RequestIdPB the results for the call will be tracked and the call may have exactly once semantics. This allows to have the simplest form of exactly once semantics for single server calls. Of course limitations apply, like response persistence across restarts is not supported, neither is propagating/rebuilding responses to/on replicas for replicated calls. If support for this is needed it will have to be done ad-hoc, case by case. Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 --- M src/kudu/common/common.proto M src/kudu/rpc/CMakeLists.txt M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/protoc-gen-krpc.cc A src/kudu/rpc/rpc-stress-test.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rtest.proto M src/kudu/rpc/service_if.cc M src/kudu/rpc/service_if.h 11 files changed, 323 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3192/1 -- To view, visit http://gerrit.cloudera.org:8080/3192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves
