[kudu-CR] Integrate the ResultTracker into the rpc subsystem

2016-07-14 Thread David Ribeiro Alves (Code Review)
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

2016-07-14 Thread Jean-Daniel Cryans (Code Review)
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

2016-07-12 Thread Kudu Jenkins (Code Review)
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

2016-07-12 Thread David Ribeiro Alves (Code Review)
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

2016-07-12 Thread David Ribeiro Alves (Code Review)
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

2016-07-12 Thread Todd Lipcon (Code Review)
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

2016-07-12 Thread David Ribeiro Alves (Code Review)
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

2016-07-12 Thread David Ribeiro Alves (Code Review)
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

2016-07-12 Thread Kudu Jenkins (Code Review)
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

2016-07-12 Thread Kudu Jenkins (Code Review)
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

2016-07-12 Thread David Ribeiro Alves (Code Review)
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

2016-07-12 Thread David Ribeiro Alves (Code Review)
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

2016-07-08 Thread Kudu Jenkins (Code Review)
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

2016-07-08 Thread Kudu Jenkins (Code Review)
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

2016-07-06 Thread Todd Lipcon (Code Review)
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

2016-07-06 Thread David Ribeiro Alves (Code Review)
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

2016-07-06 Thread Kudu Jenkins (Code Review)
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

2016-07-06 Thread David Ribeiro Alves (Code Review)
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

2016-07-05 Thread Kudu Jenkins (Code Review)
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

2016-06-30 Thread David Ribeiro Alves (Code Review)
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

2016-06-30 Thread David Ribeiro Alves (Code Review)
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

2016-06-30 Thread Kudu Jenkins (Code Review)
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

2016-06-27 Thread David Ribeiro Alves (Code Review)
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

2016-06-27 Thread Kudu Jenkins (Code Review)
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

2016-06-27 Thread Kudu Jenkins (Code Review)
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

2016-06-27 Thread Kudu Jenkins (Code Review)
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

2016-06-27 Thread David Ribeiro Alves (Code Review)
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

2016-06-27 Thread Kudu Jenkins (Code Review)
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

2016-06-27 Thread Kudu Jenkins (Code Review)
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

2016-06-27 Thread David Ribeiro Alves (Code Review)
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

2016-06-26 Thread Kudu Jenkins (Code Review)
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

2016-06-22 Thread David Ribeiro Alves (Code Review)
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

2016-06-22 Thread David Ribeiro Alves (Code Review)
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

2016-06-22 Thread Kudu Jenkins (Code Review)
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

2016-06-22 Thread Todd Lipcon (Code Review)
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

2016-06-21 Thread Adar Dembo (Code Review)
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

2016-06-20 Thread Kudu Jenkins (Code Review)
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

2016-06-20 Thread Kudu Jenkins (Code Review)
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

2016-06-20 Thread David Ribeiro Alves (Code Review)
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

2016-06-20 Thread David Ribeiro Alves (Code Review)
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

2016-06-20 Thread Kudu Jenkins (Code Review)
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

2016-06-20 Thread David Ribeiro Alves (Code Review)
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

2016-06-20 Thread Kudu Jenkins (Code Review)
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

2016-05-24 Thread David Ribeiro Alves (Code Review)
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

2016-05-24 Thread Adar Dembo (Code Review)
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

2016-05-24 Thread David Ribeiro Alves (Code Review)
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

2016-05-24 Thread David Ribeiro Alves (Code Review)
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

2016-05-24 Thread David Ribeiro Alves (Code Review)
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